warning: comparison of integer expressions of different signedness

[fujitatomoya]

address #2167 (comment)

[fujitatomoya]

The related code was developed in #2164 and backport humble #2167 detected #2167 (comment)

In file included from /tmp/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:20:
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h: In instantiation of ‘testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = unsigned int; T2 = int]’:
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1554:23:   required from ‘static testing::AssertionResult testing::internal::EqHelper::Compare(const char*, const char*, const T1&, const T2&) [with T1 = unsigned int; T2 = int; typename std::enable_if<((! std::is_integral<_Tp>::value) || (! std::is_pointer<_Dp>::value))>::type* <anonymous> = 0]’
/tmp/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:676:3:   required from ‘void TestIntraprocessExecutors_testIntraprocessRetrigger_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = rclcpp::executors::MultiThreadedExecutor]’
/tmp/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:654:1:   required from here
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1527:11: warning: comparison of integer expressions of different signedness: ‘const unsigned int’ and ‘const int’ [-Wsign-compare]
 1527 |   if (lhs == rhs) {
      |       ~~~~^~~~~~
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h: In instantiation of ‘testing::AssertionResult testing::internal::CmpHelperEQ(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]’:
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1554:23:   required from ‘static testing::AssertionResult testing::internal::EqHelper::Compare(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int; typename std::enable_if<((! std::is_integral<_Tp>::value) || (! std::is_pointer<_Dp>::value))>::type* <anonymous> = 0]’
/tmp/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:697:3:   required from ‘void TestIntraprocessExecutors_testIntraprocessRetrigger_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = rclcpp::executors::MultiThreadedExecutor]’
/tmp/ws/src/ros2/rclcpp/rclcpp/test/rclcpp/executors/test_executors.cpp:654:1:   required from here
/tmp/ws/install_isolated/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:1527:11: warning: comparison of integer expressions of different signedness: ‘const long unsigned int’ and ‘const int’ [-Wsign-compare]

i think it would be nicer to address this rolling then backporting to iron and humble.

[fujitatomoya]

I was trying to confirm the original warning but could not.

with colcon test --event-handlers console_direct+ --abort-on-error --packages-select rclcpp --ctest-args -R test_executors, it goes 100% tests passed with this PR.

[clalancette]

This is really weird. We have both humble and rolling on the same underlying OS (Ubuntu 22.04), with the same code in place, and one of them has the warning and one does not. I'm honestly not sure what is going on with that, but we should move forward with a fix.

This fix looks generally correct to me, though I'll also say that we should change everywhere that deals with callback_count to use unsigned. So in the SetUp, we should assign it to 0u, in the callback we should do fetch_add(1u), and when we reset the counter we should do store(0u). Once those are fixed, and CI is green, I'm happy to approve.

[fujitatomoya]

This is really weird. We have both humble and rolling on the same underlying OS (Ubuntu 22.04), with the same code in place, and one of them has the warning and one does not.

that is what i though too. not sure who this can be detected by CI, cz i cannot reproduce this warning with humble source build.
could be related to #2121? and so #2167 generates the warning?

though I'll also say that we should change everywhere that deals with callback_count to use unsigned.

makes sense to me. will do that.

[clalancette]

that is what i though too. not sure who this can be detected by CI, cz i cannot reproduce this warning with humble source build.

Oh, I can reproduce the warning with humble locally. But I cannot reproduce it with rolling, which is why I think this is super weird.

Regardless, I'm fine with your change here (for both Rolling and Humble), so we can go forward and get this in and then backport it.

[fujitatomoya]

CI:

• Linux
• Linux-aarch64
• Windows

[clalancette]

Looks good to me with green CI.

[fujitatomoya]

@Mergifyio backport to iron humble

[mergify]

backport to iron humble

❌ No backport have been created

• Backport to branch to failed

GitHub error: Branch not found

• Backport to branch iron in progress
• Backport to branch humble in progress

[fujitatomoya]

@Mergifyio backport iron humble

[mergify]

backport iron humble

✅ Backports have been created

• #2222 warning: comparison of integer expressions of different signedness (backport #2219) has been created for branch iron
• #2223 warning: comparison of integer expressions of different signedness (backport #2219) has been created for branch humble