[rclcpp_action] Add warnings

[audrow]

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]

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

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

Looks generally good to me once the CI failures are sorted out.

[clalancette]

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]

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

Looks good with green CI.

[audrow]

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]

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

[clalancette]

I left a few comments (mostly notes to myself), and one question. Once that question is answered, I'm happy to approve.

[clalancette]

Looks good with green CI.

[audrow]

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

[clalancette]

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]

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]

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]

• Linux
• Linux-aarch64
• macOS
• Windows