Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix last clang-tidy warnings #11

Open
GretaCB opened this issue Aug 22, 2018 · 1 comment
Open

Fix last clang-tidy warnings #11

GretaCB opened this issue Aug 22, 2018 · 1 comment

Comments

@GretaCB
Copy link
Contributor

GretaCB commented Aug 22, 2018

Per @springmeyer and @joto 's thoughts:

We have a few clang-tidy warnings needing fixed.

They should be failing the build, but are not due to mapbox/node-cpp-skel#105.

These are what show up for me with make tidy locally on OS X:

../src/filters.cpp:50:13: error: initializing non-owner 'Filters *const' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory,-warnings-as-errors]
            auto* const self = new Filters();
            ^
../src/shave.cpp:152:9: error: initializing non-owner 'AsyncBaton *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory,-warnings-as-errors]
        auto* baton = new AsyncBaton();
        ^
../src/shave.cpp:342:60: error: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory,-warnings-as-errors]
                                                           delete reinterpret_cast<std::string*>(hint);
                                                           ^
../src/shave.cpp:341:66: note: variable declared here
                                                       [](char*, void* hint) {
                                                                 ^
../src/shave.cpp:353:5: error: deleting a pointer through a type that is not marked 'gsl::owner<>'; consider using a smart pointer instead [cppcoreguidelines-owning-memory,-warnings-as-errors]
    delete baton;
    ^
../src/shave.cpp:329:5: note: variable declared here
    auto* baton = static_cast<AsyncBaton*>(req->data);
    ^

Now, I noticed that @joto just disabled this clang-tidy warning (`cppcoreguidelines-owning-memory) today in vtzero which indicates to be we may want to consider the same thing: mapbox/vtzero@6822c59


I think the cppcoreguidelines-owning-memory only makes sense if you buy into the whole gsl library. Each project has to decide whether it makes sense for them, but I think the gsl library makes more sense for higher level applications while lower-level libraries a) need more bit-twiddling and pointer-twiddling etc. anyway so they need to disable lots of these warnings and b) any extra dependency on low-level libraries makes they use that much harder.

@springmeyer
Copy link
Contributor

Several of these warnings @alliecrevier fixed in node-cpp-skel as well: mapbox/node-cpp-skel#129. So we can learn from those commits to address these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants