vtshaver icon indicating copy to clipboard operation
vtshaver copied to clipboard

Fix last clang-tidy warnings

Open GretaCB opened this issue 7 years ago • 1 comments
trafficstars

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 https://github.com/mapbox/node-cpp-skel/issues/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: https://github.com/mapbox/vtzero/commit/6822c59e17a6d51196d78fce6d2a5d383522c6e9


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.

GretaCB avatar Aug 22 '18 23:08 GretaCB

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

springmeyer avatar Aug 23 '18 00:08 springmeyer