clang-tidy error around std::string& member

@GretaCB

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.

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 6be05eb 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.

try alternative method to trigger #65 - is this caught by asan?

fix clang-tidy warning #65 of [google-runtime-member-string-references]

Win. Moving to a pointer in da9ea7b 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

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");
                  ^

1f4d34a fixes the -Werror=effc++ error and 278aae5 avoids the cert-err60-cpp issue.

Will be fixed by #66

Navigation Menu

Repository navigation

clang-tidy error around std::string& member #65

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

clang-tidy error around std::string& member #65

Description

Activity

springmeyer commented on Aug 17, 2017

springmeyer commented on Aug 18, 2017

springmeyer commented on Aug 18, 2017

springmeyer commented on Aug 18, 2017

springmeyer commented on Aug 18, 2017

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions