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.

Sign up for GitHub

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

Jump to bottom

Add thread annotation macros #2

Merged
merged 3 commits into from
Mar 15, 2019
Merged

Add thread annotation macros #2

merged 3 commits into from
Mar 15, 2019

Conversation

emersonknapp
Copy link
Contributor

@emersonknapp emersonknapp commented Feb 26, 2019 •
edited
Loading

For clang thread safety analysis https://clang.llvm.org/docs/ThreadSafetyAnalysis.html

Related to ros2/ros2#664

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 5d47939 to 42f6d28 Compare February 26, 2019 18:50
thomas-moulard
thomas-moulard approved these changes Feb 26, 2019
View reviewed changes
Copy link

@thomas-moulard thomas-moulard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mainly LGTM, we should probably have a no-op test which is just including the header so that we can detect compilation errors in this package w/o having to pull that in a dependency and discover there that it actually does not work.

CHANGELOG.rst Outdated Show resolved Hide resolved
package.xml Outdated Show resolved Hide resolved
@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch 2 times, most recently from cc02ae9 to 720c5be Compare February 26, 2019 18:55
@thomas-moulard
Copy link

Awesome - SGTM!

I think it's good we don't force everyone to compile with -D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS but we should also try not to forget to pass that in each package we convert as needed...

@emersonknapp emersonknapp mentioned this pull request Feb 26, 2019
Merged
@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 8819e9f to 87820aa Compare February 27, 2019 00:12
@thomas-moulard
Copy link

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@thomas-moulard thomas-moulard mentioned this pull request Feb 27, 2019
Closed
3 tasks
tfoote
tfoote reviewed Feb 27, 2019
View reviewed changes
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit more documentation would be great. And it looks like we're only at ~25% coverage of the macros in the test. It would be good to try to increase that at least a little bit as I think getting to at least the majority of them would be relatively straight forward.

package.xml Outdated Show resolved Hide resolved
@emersonknapp
Copy link
Contributor Author

I'll make a go at hitting all the macros in unit test. The first pass was mainly to make sure that including the header doesn't break the build

@emersonknapp emersonknapp self-assigned this Mar 7, 2019
@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from e37de87 to 888493c Compare March 8, 2019 03:43
@emersonknapp
Copy link
Contributor Author

Updated the documentation, and updated tests to cover as many of the macros as possible, and left detailed analysis/documentation of the ones that could not be tested.

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 31b92f8 to 7f507db Compare March 8, 2019 04:32
@emersonknapp emersonknapp mentioned this pull request Mar 8, 2019
Merged
@emersonknapp
Copy link
Contributor Author

Any feedback on latest revision?

@emersonknapp emersonknapp mentioned this pull request Mar 13, 2019
Merged
tfoote
tfoote approved these changes Mar 13, 2019
View reviewed changes
Copy link
Contributor

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. The changes lgtm. Starting CI

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

Latest commit should fix CI issues - was able to reproduce locally by doing clang w/ libstdc++ - I had been only build gcc and clang+libcxx on the latest versions

@tfoote
Copy link
Contributor

tfoote commented Mar 14, 2019

Retriggered CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@emersonknapp
Copy link
Contributor Author

@tfoote with the build fix, are we ok to merge this?

@wjwwood
Copy link
Member

wjwwood commented Mar 15, 2019

All commits have to be signed-off, the DCO check is currently failing.

@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 093685a to 0840c25 Compare March 15, 2019 17:38
Emerson Knapp added 3 commits March 15, 2019 18:00
Add thread annotation macros, tests for same, and documentation
15e56c3 
Signed-off-by: Emerson Knapp <[email protected]>
Change macro namespace to RCPPUTILS_TSA
cadd706 
Signed-off-by: Emerson Knapp <[email protected]>
Allow build for clang w/ libstdc++, add test for NO_THREAD_SAFETY_ANA…
d3c1c8a 
…LYSIS, and fix argument count of ASSERT_CAPABILITY macros

Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp force-pushed the thread-annotation-macros branch from 0840c25 to d3c1c8a Compare March 15, 2019 18:00
@emersonknapp emersonknapp merged commit 028a6cf into ros2:master Mar 15, 2019
@tfoote tfoote removed the in review label Mar 15, 2019
@emersonknapp emersonknapp deleted the thread-annotation-macros branch March 15, 2019 18:18
@emersonknapp emersonknapp mentioned this pull request Sep 5, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood left review comments

@thomas-moulard thomas-moulard thomas-moulard approved these changes

@tfoote tfoote tfoote approved these changes

Assignees

@emersonknapp emersonknapp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emersonknapp @thomas-moulard @tfoote @wjwwood