-
Notifications
You must be signed in to change notification settings - Fork 10
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
Comments
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:
It is crazy that AddressSanitizer does not detect this. Even though we are using Results at https://travis-ci.org/mapbox/node-cpp-skel/builds/265783690 Detailed error on linux builds:
|
Win. Moving to a pointer in da9ea7b lead to this new warning being caught by the travis job that uses the
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:
|
Will be fixed by #66 |
While enabling clang-tidy support (#63 and #64) this error came up:
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. Thename
is owned by theHelloObjectAsync
class here and is only temporarily neede by the theAsyncHelloWorker
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.
The text was updated successfully, but these errors were encountered: