mapbox / node-cpp-skel

Skeleton for bindings to C++ libraries for Node.js using node-addon-api
Creative Commons Zero v1.0 Universal
72 stars 10 forks source link

make tidy removes empty initializers and leaves commas #86

Closed mapsam closed 7 years ago

mapsam commented 7 years ago

Here's an example struct with two empty initializers name() and type():

struct Thing {
    Thing(std::string msg)
      : message(msg),
        name(),
        type() {}

    std::string message;
    std::string name;
    std::string type;
}

And when running make tidy the output looks like this:

struct Thing {
    explicit Thing(std::string msg)
      : message(std::move(msg)),
        , // <--- what are you doing here??
        {}

    std::string message;
    std::string name;
    std::string type;
};

Notice the dangling and extra commas , - these result in compiler errors. Perhaps there is a way to tell clang tidy to disregard empty initializers?

cc @GretaCB @mapbox/core-tech

springmeyer commented 7 years ago

Thanks for creating. Love how node-cpp-skel can be the place to discuss and resolve oddities like this so we can dodge them everywhere (once fixed).

This problem seems like a clang-tidy bug. So I wonder if upgrading the llvm/clang++/clang-tidy version would fix it (We recently did this upgrade in skel). So if you start pulling clang-tidy 5.0 instead of 4.0 I wonder if the bug is fixed?

mapsam commented 7 years ago

Downloading clang-tidy 5.0.0 doesn't seem to solve the problem, unfortunately ._.

Downloading binary package osx-x86_64/clang-tidy/5.0.0.tar.gz...
springmeyer commented 7 years ago

Run ‘make distclean’ to clear out the .toolchain folder to ensure it is being used. But yeah, if that does not fix then I’ll think of something else.

mapsam commented 7 years ago

doh! That does seem to fix the issue @springmeyer.

springmeyer commented 7 years ago

Oh yay. Note: when I updated node-cpp-skel there were some minor differences in format output: so had to re-run that too.

springmeyer commented 7 years ago

Closing. Resolution is to use clang++ 5.x. For projects based on skel that means ensuring we are pulling >= LLVM 5.0.0 at https://github.com/mapbox/node-cpp-skel/blob/e54f258a67c4252137617f851bf98d503aa5dd9f/scripts/setup.sh#L7