Skip to content

[dashing backport] Fix conversion of negative durations to messages (#1188)#1207

Merged
hidmic merged 2 commits intodashingfrom
hidmic/dashing/backport-1188
Oct 5, 2020
Merged

[dashing backport] Fix conversion of negative durations to messages (#1188)#1207
hidmic merged 2 commits intodashingfrom
hidmic/dashing/backport-1188

Conversation

@hidmic
Copy link
Copy Markdown
Contributor

@hidmic hidmic commented Jun 30, 2020

  • Fix conversion from negative Duration or Time to the respective message type and throw in Duration::to_rmw_time() if the duration is negative.
    rmw_time_t cannot represent negative durations.

Constructors and assignment operators can be just defaulted.

Other changes are mainly cosmetical, to make conversions between signed
and unsigned types and between 32-bit and 64-bit types more explicit.

Signed-off-by: Johannes Meyer johannes@intermodalics.eu

  • Add -Wconversion compiler option and fix implicit conversions that might alter the value

Signed-off-by: Johannes Meyer johannes@intermodalics.eu

  • Fix usage of fixture class in some unit tests by using gtest macro TEST_F() instead of TEST().

Signed-off-by: Johannes Meyer johannes@intermodalics.eu

  • Add compiler option -Wno-sign-conversion to fix build with Clang on macOS

Signed-off-by: Johannes Meyer johannes@intermodalics.eu

Copy link
Copy Markdown
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Thanks for doing the backport!

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Jul 1, 2020

Alright, the extra warnings are due to the addition of -Wconversion plus #901 being missing in dashing. It looks API & ABI compatible to me, so I'll backport that one. Then, the PR job here will go green.

@ivanpauno
Copy link
Copy Markdown
Member

Alright, the extra warnings are due to the addition of -Wconversion plus #901 being missing in dashing. It looks API & ABI compatible to me, so I'll backport that one. Then, the PR job here will go green.

You can either backport #901 or delete the addition of the new warning.
Though it's great to have a new check on master, in branches of previous releases the addition doesn't add much value.

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Jul 1, 2020

You can either backport #901 or delete the addition of the new warning.

I'd rather fix the problem than turn off the alarm. Some of the casts in #901 do have an effect besides shutting up the compiler.

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Jul 1, 2020

Depends on #1209.

meyerj and others added 2 commits September 17, 2020 20:27
* Fix conversion from negative Duration or Time to the respective message type and throw in Duration::to_rmw_time() if the duration is negative.
rmw_time_t cannot represent negative durations.

Constructors and assignment operators can be just defaulted.

Other changes are mainly cosmetical, to make conversions between signed
and unsigned types and between 32-bit and 64-bit types more explicit.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Add -Wconversion compiler option and fix implicit conversions that might alter the value

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Fix usage of fixture class in some unit tests by using gtest macro TEST_F() instead of TEST().

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Add compiler option -Wno-sign-conversion to fix build with Clang on macOS

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/dashing/backport-1188 branch from ef389e7 to 9481782 Compare September 17, 2020 23:27
@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Sep 17, 2020

CI up rclcpp:

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

@hidmic
Copy link
Copy Markdown
Contributor Author

hidmic commented Oct 5, 2020

Test failures are unrelated to this patch. Merging.

@hidmic hidmic merged commit 1f0f8f6 into dashing Oct 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/dashing/backport-1188 branch October 5, 2020 19: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.

4 participants