-
Notifications
You must be signed in to change notification settings - Fork 412
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 virtual dispatch issues identified by clang-tidy #1816
Conversation
The changes look good to me. |
We moved the issue to a new place, which should be accessible. space-ros/space-ros#8 |
53ed68b
to
4369a9f
Compare
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.
There are some changes that doesn't seem directly related to the PR.
I have left some comments in those.
If they aren't related, maybe we can open new PRs for them (?).
Otherwise LGTM!
I think I addressed all your comments with just replies, but please have another look when you can. |
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.
lgtm
ac424bc
to
b4e8de3
Compare
I still need to fix a new compiler warning on Windows here. |
b4e8de3
to
480d527
Compare
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
0906a3a
to
a84ad74
Compare
Signed-off-by: William Woodall <william@osrfoundation.org>
The Windows test failure is unrelated and flaky. |
Signed-off-by: William Woodall <william@osrfoundation.org>
The test timeout in Rpr is unrelated. |
…)" This reverts commit dbded5c. I'm just doing this to test a build.
fixes space-ros/space-ros#8
I might come back and try to de-duplicate some of the code, but I couldn't find a nice solution yet.