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

[rclcpp_action] Add warnings #1405

Merged
merged 7 commits into from
Dec 19, 2020
Merged

[rclcpp_action] Add warnings #1405

merged 7 commits into from
Dec 19, 2020

Conversation

audrow
Copy link
Member

@audrow audrow commented Oct 14, 2020

This PR enables -Wformat=2, -Wconversion, -Wshadow, -Wsign-conversion, and -Wcast-qual in rclcpp_action. This PR relies on using gtest v1.10.0, see ament/googletest#8.

@audrow audrow self-assigned this Oct 14, 2020
@audrow
Copy link
Member Author

audrow commented Oct 14, 2020 •
edited
Loading

Here's CI. I'll look into why rcl is failing to build.

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

clalancette
clalancette reviewed Oct 14, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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) {
Copy link
Contributor

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) {

@audrow
Copy link
Member Author

audrow commented Oct 21, 2020

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

@audrow audrow force-pushed the audrow/add-warnings-rclcpp_action branch from f7093d3 to 0287c26 Compare October 29, 2020 18:48
clalancette
clalancette approved these changes Oct 29, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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.

@audrow
Copy link
Member Author

audrow commented Oct 29, 2020

Using uint32_t in the for loop above seems to introduce a warning in the PR job (and maybe in CI, if I reran it). Should I revert back?

/tmp/ws/src/rclcpp/rclcpp_action/test/test_client.cpp:137:52: warning: conversion to ‘std::vector<int>::size_type’ {aka ‘long unsigned int’} from ‘int32_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
  137 |                 feedback_message.feedback.sequence[i] +
      |                                                    ^
/tmp/ws/src/rclcpp/rclcpp_action/test/test_client.cpp:138:54: warning: conversion to ‘std::vector<int>::size_type’ {aka ‘long unsigned int’} from ‘int32_t’ {aka ‘int’} may change the sign of the result [-Wsign-conversion]
  138 |                 feedback_message.feedback.sequence[i - 1]);
      |                                                    ~~^~~

@clalancette
Copy link
Contributor

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 std::vector that comprises the feeback sequence (std::vector::size_type, aka size_t). (see https://en.cppreference.com/w/cpp/container/vector for more information)

My suggestion to resolve it is the following:

for (size_t i = 1; i < static_cast<size_t>(goal_request->goal.order); ++i) {

@audrow audrow mentioned this pull request Nov 5, 2020
Merged
@audrow audrow force-pushed the audrow/add-warnings-rclcpp_action branch from 0287c26 to d79baff Compare December 15, 2020 18:05
clalancette
clalancette reviewed Dec 15, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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.

rclcpp_action/test/benchmark/benchmark_action_client.cpp Show resolved Hide resolved
rclcpp_action/test/benchmark/benchmark_action_client.cpp Show resolved Hide resolved
rclcpp_action/test/benchmark/benchmark_action_client.cpp Show resolved Hide resolved
rclcpp_action/test/test_client.cpp Show resolved Hide resolved
clalancette
clalancette approved these changes Dec 16, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a 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.

@audrow
Copy link
Member Author

audrow commented Dec 17, 2020 •
edited
Loading

Great, I'll rerun CI after #1442 is merged in.

@clalancette
Copy link
Contributor

Great, I'll rerun CI after #1442 is merged in.

For what it's worth, I don't think there is anything in this PR that depends on #1442 getting in. So if CI is green with just this change, I'd say go ahead and merge it.

@audrow
Copy link
Member Author

audrow commented Dec 17, 2020 •
edited
Loading

For what it's worth, I don't think there is anything in this PR that depends on #1442 getting in. So if CI is green with just this change, I'd say go ahead and merge it.

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.

@clalancette
Copy link
Contributor

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 :).

@audrow
Add warnings
f21d70f 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Simplify for loop in test_client.cpp
855bed6 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Fix conversion warning in test_client static cast to size_t
703107f 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Fix new warnings after rebasing on master
6e99f6e 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Fix shadowing in the benchmark action server
b35ece3 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Static cast goal order to size_t
334d542 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Remove unnecessary include
551330a 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow audrow force-pushed the audrow/add-warnings-rclcpp_action branch from 9bfb9c5 to 551330a Compare December 19, 2020 04:32
@audrow
Copy link
Member Author

audrow commented Dec 19, 2020

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

@audrow audrow merged commit 543a3c3 into master Dec 19, 2020
@delete-merged-branch delete-merged-branch bot deleted the audrow/add-warnings-rclcpp_action branch December 19, 2020 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

Assignees

@audrow audrow

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@audrow @clalancette