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.

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

Removed test_timers_manager clang warning #2479

Merged

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Apr 3, 2024

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde ahcorde self-assigned this Apr 3, 2024
@@ -375,7 +375,7 @@ TEST_F(TestTimersManager, check_one_timer_cancel_doesnt_affect_other_timers)
// since typical users aren't going to mess around with the timer manager.
t1 = TimerT::make_shared(
1ms,
[&t1_runs, &t1, cancel_iter]() {
[&t1_runs, &t1]() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I knew this, but had to look it up again.

https://en.cppreference.com/w/cpp/language/lambda

A lambda expression can read the value of a variable without capturing it if the variable has const non-volatile integral or enumeration type and has been initialized with a constant expression

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea :). Today I learned something :).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MSVC doesn't seem to like it, though:

**09:25:11 C:\ci\ws\src\ros2\rclcpp\rclcpp\test\rclcpp\test_timers_manager.cpp(380,1): error C3493: 'cancel_iter' cannot be implicitly captured because no default capture mode has been specified [C:\ci\ws\build\rclcpp\test\rclcpp\test_timers_manager.vcxproj]
**

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we set the default capture mode as it says?

Suggested change
[&t1_runs, &t1]() {
[=, &t1_runs, &t1]() {

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.

@@ -375,7 +375,7 @@ TEST_F(TestTimersManager, check_one_timer_cancel_doesnt_affect_other_timers)
// since typical users aren't going to mess around with the timer manager.
t1 = TimerT::make_shared(
1ms,
[&t1_runs, &t1, cancel_iter]() {
[&t1_runs, &t1]() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea :). Today I learned something :).

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2024

  • Linux Build Status

  • Linux-aarch64 Build Status

  • Windows Build Status

  • Linux clang Build Status

  • Linux-aarch64 clang Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2024

Windows doesn't like the change

error C3493: 'cancel_iter' cannot be implicitly captured because no default capture mode has been specified [C:\ci\ws\build\rclcpp\test\rclcpp\test_timers_manager.vcxproj]

@clalancette
Copy link
Contributor

Windows doesn't like the change

See our conversation in #2479 (comment)

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 3, 2024

  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 4, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status
  • Linux Build Status
  • Linux-aarch64 clang Build Status

Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Apr 4, 2024

This specific warning is removed from the clang builds, Merging

@ahcorde ahcorde merged commit f7a7954 into rolling Apr 4, 2024
2 of 3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/rolling/remove_test_timers_manager_clang_warning branch April 4, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants