Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
springmeyer opened this issue Aug 17, 2017 · 5 comments
Closed

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

springmeyer opened this issue Aug 17, 2017 · 5 comments

Comments

@springmeyer
Copy link
Contributor

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
Copy link
Contributor Author

springmeyer commented Aug 17, 2017

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.

@springmeyer
Copy link
Contributor Author

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

@springmeyer
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@springmeyer
Copy link
Contributor Author

Will be fixed by #66

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant