clang-tidy error around std::string& member

[springmeyer]

While enabling clang-tidy support (#63 and #64) this error came up:

../src/object_async/hello_async.cpp:210:5: error: const string& members are dangerous; it is much better to use alternatives, such as pointers or simple constants [google-runtime-member-string-references,-warnings-as-errors]
    std::string const& name_;
    ^

And I found: https://clang.llvm.org/extra/clang-tidy/checks/google-runtime-member-string-references.html

This is interesting/surprising. I've used const std::string & members before and gotten away clean, I think. The motivation here for @GretaCB and I was to ensure we did not need to copy this string when going into the threadpool. The name is owned by the HelloObjectAsync class here and is only temporarily neede by the the AsyncHelloWorker that takes the const reference and keeps it as a member here.

This warning indicates that member references are worst than a pointer, which would achieve the same purpose. So I'm going to look into using a pointer.

[springmeyer]

WOW. Okay, learning a lot here. The docs at https://clang.llvm.org/extra/clang-tidy/checks/google-runtime-member-string-references.html are clear that this is trouble because you could easily, accidentally, send a temporarily into the class and the code would still compile and run. Testing this simulated accident (like at #40) with https://github.com/mapbox/node-cpp-skel/commit/6be05eb56e35d1ce9341438b6fa474ad897d3b87 gives:

	OS X/local Release	Asan linux/travis	Linuxrelease	Linux release
Status	All tests passed 😨	All tests passed 😨	Tests failed with segfault or Error: basic_string::_S_create ✅	Tests failed with segfault or Error: basic_string::_S_create ✅

It is crazy that AddressSanitizer does not detect this. Even though we are using ASAN_OPTIONS=detect_stack_use_after_return=1 and -fsanitize-address-use-after-scope in setup.sh like recommended at https://stackoverflow.com/a/42342428.

Results at https://travis-ci.org/mapbox/node-cpp-skel/builds/265783690

Detailed error on linux builds:

# success: prints expected string via custom constructor
/home/travis/build/mapbox/node-cpp-skel/test/hello_object_async.test.js:7
    if (err) throw err;
             ^
Error: basic_string::_S_create
    at Error (native)
npm ERR! Test failed.  See above for more details.

[springmeyer]

Win. Moving to a pointer in https://github.com/mapbox/node-cpp-skel/commit/da9ea7bb5b0e4714743696b89be6a00dbeac8e5a lead to this new warning being caught by the travis job that uses the -Weffc++ warning flat:

../src/object_async/hello_async.cpp:164:8: error: ‘struct object_async::AsyncHelloWorker’ has pointer data members [-Werror=effc++] struct AsyncHelloWorker : Nan::AsyncWorker ^~~~~~~~~~~~~~~~ ../src/object_async/hello_async.cpp:164:8: error: but does not override ‘object_async::AsyncHelloWorker(const object_async::AsyncHelloWorker&)’ [-Werror=effc++] ../src/object_async/hello_async.cpp:164:8: error: or ‘operator=(const object_async::AsyncHelloWorker&)’ [-Werror=effc++] cc1plus: all warnings being treated as errors

https://travis-ci.org/mapbox/node-cpp-skel/jobs/265788184#L657

[springmeyer]

Also seeing one error on linux that I am not seeing locally on OS X:

../src/object_async/hello_async.cpp:144:19: error: thrown exception type is not nothrow copy constructible [cert-err60-cpp,-warnings-as-errors]
            throw std::runtime_error("Uh oh, this should never happen");
                  ^

[springmeyer]

https://github.com/mapbox/node-cpp-skel/commit/1f4d34aa7a6f027f4ce11313d23cfd2634b758e0 fixes the -Werror=effc++ error and https://github.com/mapbox/node-cpp-skel/commit/278aae5d714afe4495d4adfe442ee76fcca9889e avoids the cert-err60-cpp issue.

[springmeyer]

Will be fixed by #66