Upgrade to llvm 6.0.0 redux

[springmeyer]

@allieoop packaged the freshly released LLVM 6.0.0 toolchain in mason. This updates node-cpp-skel to start using (and testing) it. If this works then all downstream deps will be encouraged to update too (by forward porting to the latest skel framework).

This change specifically:

• Uses new packages for llvm 6.0.0 (and sub-packages) mapbox/mason#600 (@allieoop)
• Uses bundled pyyaml such that, when using >= llvm 6, we do not need to manually install an external pyyaml to be able to use run-clang-tidy.py (refs mapbox/node-cpp-skel#106)

Note: this PR replaces https://github.com/mapbox/node-cpp-skel/pull/118

/cc @allieoop

[springmeyer]

This is currently blocked by:

 ./mason_packages; ./node_modules/.bin/mason-js link
info Symlinked:  /Users/dane/projects/node-cpp-skel/mason_packages/headers/protozero/1.6.1 to  /Users/dane/projects/node-cpp-skel/mason_packages/.link
ERR! Error: EACCES: permission denied, unlink '/Users/dane/projects/node-cpp-skel/mason_packages/.link/include/c++/v1/__bit_reference' 
make[1]: *** [mason_packages/.link] Error 1
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2

Which is ticketed at https://github.com/mapbox/mason-js/issues/58. In short, this is not a problem with LLVM 6.0.0, but appears to be a problem in mason-js that surfaces with LLVM 6.0.0.

[springmeyer]

Once the above error is solved, we'll hit 2 others related to new features of clang-tidy detecting existing issues in our code:

readability-redundant-member-init

These are captured at https://github.com/mapbox/node-cpp-skel/pull/118#issuecomment-384127157 and should be fixable with:

diff --git a/src/object_async/hello_async.cpp b/src/object_async/hello_async.cpp
index b871a41..22f66bc 100644
--- a/src/object_async/hello_async.cpp
+++ b/src/object_async/hello_async.cpp
@@ -155,7 +155,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker // NOLINT to disable cppcoreguideline

     AsyncHelloWorker(bool louder, const std::string* name,
                      Nan::Callback* cb)
-        : Base(cb), result_{}, louder_{louder}, name_{name} {}
+        : Base(cb), louder_{louder}, name_{name} {}

     // The Execute() function is getting called when the worker starts to run.
     // - You only have access to member variables stored in this worker.
@@ -186,7 +186,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker // NOLINT to disable cppcoreguideline
         callback->Call(argc, static_cast<v8::Local<v8::Value>*>(argv));
     }

-    std::string result_;
+    std::string result_{};
     const bool louder_;
     // We use a pointer here to avoid copying the string data.
     // This works because we know that the original string we are
diff --git a/src/standalone_async/hello_async.cpp b/src/standalone_async/hello_async.cpp
index 68fb652..72270ee 100644
--- a/src/standalone_async/hello_async.cpp
+++ b/src/standalone_async/hello_async.cpp
@@ -72,7 +72,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker {
     using Base = Nan::AsyncWorker;

     AsyncHelloWorker(bool louder, Nan::Callback* cb)
-        : Base(cb), result_{}, louder_{louder} {}
+        : Base(cb), louder_{louder} {}

     // The Execute() function is getting called when the worker starts to run.
     // - You only have access to member variables stored in this worker.
@@ -105,7 +105,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker {
         callback->Call(argc, static_cast<v8::Local<v8::Value>*>(argv));
     }

-    std::string result_;
+    std::string result_{};
     const bool louder_;
 };

fuchsia-default-arguments

These are discussed in https://github.com/mapbox/node-cpp-skel/pull/118#issuecomment-384128216 and should be fixable with:

diff --git a/src/object_async/hello_async.cpp b/src/object_async/hello_async.cpp
index b871a41..11022f5 100644
--- a/src/object_async/hello_async.cpp
+++ b/src/object_async/hello_async.cpp
@@ -275,7 +275,7 @@ void HelloObjectAsync::Init(v8::Local<v8::Object> target) {

     // Create the HelloObject
     auto fnTp = Nan::New<v8::FunctionTemplate>(
-        HelloObjectAsync::New); // Passing the HelloObject::New method above
+        HelloObject::New, v8::Local<v8::Value>()); // Passing the HelloObject::New method above
     fnTp->InstanceTemplate()->SetInternalFieldCount(
         1);                     // It's 1 when holding the ObjectWrap itself and nothing else
     fnTp->SetClassName(whoami); // Passing the Javascript string object above
diff --git a/src/object_sync/hello.cpp b/src/object_sync/hello.cpp
index f6df549..b9006ed 100644
--- a/src/object_sync/hello.cpp
+++ b/src/object_sync/hello.cpp
@@ -142,7 +142,7 @@ void HelloObject::Init(v8::Local<v8::Object> target) {

     // Officially create the HelloObject
     auto fnTp = Nan::New<v8::FunctionTemplate>(
-        HelloObject::New); // Passing the HelloObject::New method above
+        HelloObject::New, v8::Local<v8::Value>()); // Passing the HelloObject::New method above
     fnTp->InstanceTemplate()->SetInternalFieldCount(
         1);                     // It's 1 when holding the ObjectWrap itself and nothing else
     fnTp->SetClassName(whoami); // Passing the Javascript string object above

[sssoleileraaa]

I'm getting that one error where a make tidy removes a parameter and leaves the comma. I think we documented this issue somewhere and maybe just manually fixed it the last time it came up. Do you remember this @springmeyer?

I'll look into how we turn this check off in our .clang-tidy file.

diff --git a/src/object_async/hello_async.cpp b/src/object_async/hello_async.cpp
index b871a41..adbe8ea 100644
--- a/src/object_async/hello_async.cpp
+++ b/src/object_async/hello_async.cpp
@@ -155,7 +155,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker // NOLINT to disable cppcoreguideline

     AsyncHelloWorker(bool louder, const std::string* name,
                      Nan::Callback* cb)
-        : Base(cb), result_{}, louder_{louder}, name_{name} {}
+        : Base(cb), , louder_{louder}, name_{name} {}

     // The Execute() function is getting called when the worker starts to run.
     // - You only have access to member variables stored in this worker.
diff --git a/src/standalone_async/hello_async.cpp b/src/standalone_async/hello_async.cpp
index 68fb652..9d09105 100644
--- a/src/standalone_async/hello_async.cpp
+++ b/src/standalone_async/hello_async.cpp
@@ -72,7 +72,7 @@ struct AsyncHelloWorker : Nan::AsyncWorker {
     using Base = Nan::AsyncWorker;

     AsyncHelloWorker(bool louder, Nan::Callback* cb)
-        : Base(cb), result_{}, louder_{louder} {}
+        : Base(cb), , louder_{louder} {}

[springmeyer]

maybe just manually fixed it the last time it came up.

The manual fix I lifted into a diff above at: https://github.com/mapbox/node-cpp-skel/pull/129#issuecomment-385544411. So I recommend applying that - please go ahead and do that to this branch.

I think we documented this issue somewhere and

Not documented anywhere beyond this ticket.

But I did confirm that the problem does not happen with LLVM head (future 7.0.0) on my machine, so I think the manual fix for now is okay since we'll eventually not get bitten by it again. Do you agree? Or maybe we should document this gocha in https://github.com/mapbox/node-cpp-skel/blob/master/docs/extended-tour.md#clang-tidy?

[springmeyer]

But I did confirm that the problem does not happen with LLVM head (future 7.0.0) on my machine

mentioned at https://github.com/mapbox/node-cpp-skel/pull/118#issuecomment-384841898

[sssoleileraaa]

I agree, manually fixing the clang-tidy change is the right step since we know this won't be an issue later on when we use LLVM 7.0.0. Thanks for confirming!

[sssoleileraaa]

@springmeyer - I made the change we discussed above and now I think this change is ready to be merged in.

Here's the official 👍

[springmeyer]

@allieoop this looks good. One thing is that this branch (I think) is still hitting clang-tidy issues. They are not failing travis due to https://github.com/mapbox/node-cpp-skel/issues/105, but they are visible in the logs: https://travis-ci.org/mapbox/node-cpp-skel/jobs/388585932#L1095 (and if you run locally). I'm 👍 on merging and then ticketing a followup on fixing the clang-tidy issues. What do you think?

[sssoleileraaa]

Yes, I agree this can be merged as we continue a conversation about #105 (I left some comments there). But before merging I'd like to confirm that:

1) We're both seeing 6 warnings being treated as errors, even though clang-tidy only acknowledges 3 of them.

The 6 errors (originally warnings) that I see are:

hello.cpp

• error: calling a function that uses a default argument is disallowed

• error: initializing non-owner 'object_sync::HelloObject *const' with a newly created 'gsl::owner<>'

  hello_async.cpp:

• error: calling a function that uses a default argument is disallowed
• error: initializing non-owner 'object_async::HelloObjectAsync *const' with a newly created 'gsl::owner<>'
• error: initializing non-owner 'object_async::AsyncHelloWorker *' with a newly created 'gsl::owner<>'

• error: initializing non-owner 'standalone_async::AsyncHelloWorker *' with a newly created 'gsl::owner<>'

  Yet clang-tidy prints:

  3 warnings treated as errors

  Do you know why only 3 are acknowledged in that statement? To make that there were actually 6 warnings, and not 3 warnings and 3 errors, I removed the line in our .clang-tidy file to treat warnings as errors and all 6 of those errors are indeed originally warnings.

2) The 6 errors are not easy to fix manually.

There are two different types of errors occurring:

1. Calling a function that uses a default argument, e.g. https://github.com/mapbox/node-cpp-skel/blob/master/src/object_sync/hello.cpp#L144 where we call Nan::New but we need to call this function. Since this is part of Nan code and outside of our control I don't see how to fix this error directly.

2. "Bad" initialization of non-owners with a newly created 'gsl::owner<>', e.g. https://github.com/mapbox/node-cpp-skel/blob/master/src/object_sync/hello.cpp#L72. Looks like this is part of clang-tidy's OwningMemoryCheck. I found some notes here: https://reviews.llvm.org/D36354 but I need more time to digest this and to figure out how a line of code like this auto* const self = new HelloObject(std::move(name)); sets it off.

[springmeyer]

Since this is part of Nan code and outside of our control I don't see how to fix this error directly.

I proposed above (see fuchsia-default-arguments) starting to be explicit and not depending on a default arg. That feels like a decent solution to me, you?

"Bad" initialization of non-owners with a newly created 'gsl::owner<>',

My feeling is that we don't want to use gls::owner and I would be up for ignoring this one in our .clang-tidy config since this is going to be very difficult to workaround (or at least I think when I've looked before it did not seem possible since we're dealing with the node API here). Maybe when we port to N-API this will go away...

[sssoleileraaa]

Your fix to the clang-tidy fuchsia-default-arguments warning worked. So it turns out that clang-tidy is fine with calling functions that have parameters with default values and only cares whether or not those default values are used. So the guideline is to always pass a value in and never rely on defaults. (This is more a note to myself.)

To address the other warning, cppcoreguidelines-owning-memory, which I think is a good guideline I switched from using raw pointers to unique_ptrs e.g.:

-                    auto* const self = new HelloObjectAsync(std::move(name));
+                    const auto self = std::make_unique<HelloObjectAsync>(std::move(name));

Update: It looks like Travis isn't finding c++14 which is why tests on Travis are failing so I'll have to into this. @springmeyer maybe you'll see what I'm doing wrong in my attempt to upgrade?

[springmeyer]

@allieoop per voice I think the std::make_unique is not being found because #include <memory> is needed. This is not erroring on OS X because one of the other includes in that file must be including <memory> already, preventing us from seeing this locally on OS X. But on linux this is not happening and surfaces the missing include. This raises the issue of how to detect these types of problems sooner and more easily. Depending on travis for catching glitches like this is okay, but not ideal since we should be able to catch it locally.

Historically I've used https://include-what-you-use.org/ for this, but recent versions have been hard to build (it is falling behind clang/llvm releases) and its somewhat painful to set up the tooling.

So, I took a quick look at whether other tools could catch this. I first tried http://clang.llvm.org/extra/include-fixer.html and it does not catch this on OS X. I presume because it sees <memory> has been included by one of the other headers and does not recommend it be included.

Then I tried cpplint, and it can catch the problem:

curl -o cpplint https://raw.githubusercontent.com/google/styleguide/gh-pages/cpplint/cpplint.py
chmod +x cpplint
$ ./cpplint src/object_async/hello_async.cpp 
src/object_async/hello_async.cpp:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
src/object_async/hello_async.cpp:4:  Found C++ system header after other header. Should be: hello_async.h, c system, c++ system, other.  [build/include_order] [4]
src/object_async/hello_async.cpp:5:  Found C++ system header after other header. Should be: hello_async.h, c system, c++ system, other.  [build/include_order] [4]
src/object_async/hello_async.cpp:6:  Found C++ system header after other header. Should be: hello_async.h, c system, c++ system, other.  [build/include_order] [4]
src/object_async/hello_async.cpp:7:  Found C++ system header after other header. Should be: hello_async.h, c system, c++ system, other.  [build/include_order] [4]
src/object_async/hello_async.cpp:54:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:58:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:70:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:86:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:87:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:87:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/object_async/hello_async.cpp:103:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:111:  Redundant blank line at the start of a code block should be deleted.  [whitespace/blank_line] [2]
src/object_async/hello_async.cpp:123:  Redundant blank line at the start of a code block should be deleted.  [whitespace/blank_line] [2]
src/object_async/hello_async.cpp:127:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:149:  { should almost always be at the end of the previous line  [whitespace/braces] [4]
src/object_async/hello_async.cpp:150:  Redundant blank line at the start of a code block should be deleted.  [whitespace/blank_line] [2]
src/object_async/hello_async.cpp:175:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:184:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:192:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:199:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:250:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:273:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:277:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:277:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/object_async/hello_async.cpp:278:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:278:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/object_async/hello_async.cpp:279:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
src/object_async/hello_async.cpp:292:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/object_async/hello_async.cpp:298:  At least two spaces is best between code and comments  [whitespace/comments] [2]
src/object_async/hello_async.cpp:86:  Add #include <utility> for move  [build/include_what_you_use] [4]
src/object_async/hello_async.cpp:250:  Add #include <memory> for make_unique<>  [build/include_what_you_use] [4]
src/object_async/hello_async.cpp:195:  Add #include <string> for string  [build/include_what_you_use] [4]
src/object_async/hello_async.cpp:298:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Done processing src/object_async/hello_async.cpp
Total errors found: 34

There is a lot of noise there, but the key is it could see the problem, on OS X, that would manifest on Linux: Add #include <memory> for make_unique<>

[springmeyer]

🙌 Congrats on landing this @allieoop - feels great to be providing more warning free, type safe, and memory-leak protected code code here, thanks to your hard work, methodical debugging, and using the latest LLVM release. The one followup I see is https://github.com/mapbox/node-cpp-skel/issues/141, which I've got on my radar for the coming days.

[springmeyer]

note: all the code improvements in this PR we kept but the actual LLVM version was reverted in #146