-
Notifications
You must be signed in to change notification settings - Fork 417
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
[rclcpp_action] Add warnings #1405
Conversation
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 generally good to me once the CI failures are sorted out.
rclcpp_action/test/test_client.cpp
Outdated
@@ -132,7 +132,7 @@ class TestClient : public ::testing::Test | |||
feedback_message.feedback.sequence.push_back(1); | |||
feedback_publisher->publish(feedback_message); | |||
client_executor.spin_once(); | |||
for (int i = 1; i < goal_request->goal.order; ++i) { | |||
for (uint32_t i = 1; i < static_cast<uint32_t>(goal_request->goal.order); ++i) { |
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.
Instead of doing the cast here, I think you could get away with:
for (int32_t i = 1; i < goal_request->goal.order; ++i) {
f7093d3
to
0287c26
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.
Looks good with green CI.
Using
|
Arg, that's annoying. In this case, we have a mismatch between the type in the struct (int32_t for goal_request->goal.order), and the indexing for the My suggestion to resolve it is the following:
|
0287c26
to
d79baff
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.
I left a few comments (mostly notes to myself), and one question. Once that question is answered, I'm happy to approve.
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 with green CI.
Great, I'll rerun CI after #1442 is merged in. |
I might be missing something, but I think CI for macOS will have warnings until I merge in and rebase on #1442. In the previous CI run, we got a bunch of nonliteral string warnings from the logging macros in this package, which should be fixed by changing the logging macros in #1442. |
Oh, you are right. Sorry, I forgot how this all fit together. Carry on :). |
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
9bfb9c5
to
551330a
Compare
This PR enables
-Wformat=2
,-Wconversion
,-Wshadow
,-Wsign-conversion
, and-Wcast-qual
inrclcpp_action
. This PR relies on using gtest v1.10.0, see ament/googletest#8.