-
Notifications
You must be signed in to change notification settings - Fork 245
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
Fix clang thread safety analysis warnings #799
Fix clang thread safety analysis warnings #799
Conversation
…s in time controller jump callbacks API. Moved cv.notify_all() inside mutex lock scope, also made method process_callback() as non-static Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
BUILD args: --packages-up-to rosbag2_cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - however I will say that these are not false positives.
cv
was used without the mutex- A static function does not have access to non-static member variables. The declaration of
static void REQUIRES(member_)
was ill-formed for requiring a non-static variable.
@emersonknapp As regards to the
Implemented workaround putting As regards to the:
Not sure if I understand you correctly but |
@emersonknapp Oh. BTW fun fact. Now we have contradiction between two tools |
That is only according to the documented usage hints, it is not a requirement of the implementation. But, we explicitly said in the code that
The |
Workaround for false positive findings by clang thread safety analysis in time controller jump callbacks API.
Address #795
Moved cv.notify_all() inside mutex lock scope, also made method process_callback() as non-static