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

Activate usual compiler warnings and fix errors #270

Merged
merged 3 commits into from
Sep 16, 2020
Merged

Activate usual compiler warnings and fix errors #270

merged 3 commits into from
Sep 16, 2020

Conversation

ivanpauno
Copy link
Member

With the same spirit that ros2/rclcpp#1149, activate usual compiler warnings.

I then fixed the errors that were reported, which included:

  • Missing virtual destructors
  • signed/unsigned comparison
  • Wrong initializer list ordering
  • Extra semicolon after method/function closing brace
  • Unused parameters

@ivanpauno ivanpauno added bug Something isn't working in review Waiting for review (Kanban column) labels Jun 1, 2020
@ivanpauno ivanpauno requested review from ahcorde and hidmic June 1, 2020 20:03
@ivanpauno ivanpauno self-assigned this Jun 1, 2020
@ivanpauno
Copy link
Member Author

@ros-pull-request-builder retest this please

hidmic
hidmic reviewed Jun 1, 2020
View reviewed changes
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor

ahcorde commented Jun 2, 2020 •
edited
Loading

There is still one ; https://github.com/ros2/geometry2/blob/ivanpauno/activate-usual-compiler-warnings-and-fix-errors/tf2/test/cache_unittest.cpp#L49

@ivanpauno
Copy link
Member Author

There is still one ; https://github.com/ros2/geometry2/blob/ivanpauno/activate-usual-compiler-warnings-and-fix-errors/tf2/test/cache_unittest.cpp#L49

Fixed in 2918e19.

@ivanpauno
Copy link
Member Author

ivanpauno commented Jun 2, 2020 •
edited
Loading

The failing test in the PR checker is also failing in other opened PRs, e.g. #269 http://build.ros2.org/job/Fpr__geometry2__ubuntu_focal_amd64/14/.

I haven't found an equivalent failure in nightlies.

@ivanpauno
Copy link
Member Author

ivanpauno commented Jun 2, 2020 •
edited
Loading

Jobs:

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

hidmic
hidmic reviewed Jun 2, 2020
View reviewed changes
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno requested a review from hidmic June 2, 2020 14:55
hidmic
hidmic approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@ivanpauno
Copy link
Member Author

Running again linux CI, that should be enough to cover the last commit changes:

  • Linux Build Status

@ivanpauno
Copy link
Member Author

@jacobperron do you prefer merging this after or before the rosdistro freeze?

There are some fixes that can't be backported (the missing virtual destructors), but I can create a separated PR with only that.

jacobperron
jacobperron requested changes Jun 2, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

I think merging the virtual destructor changes before Foxy release is relatively low risk, but I'd rather revert the changes to the logic. See my comments below.

tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
tf2/src/buffer_core.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request Jun 2, 2020
Merged
@jacobperron jacobperron mentioned this pull request Jun 3, 2020
Merged
@ivanpauno ivanpauno force-pushed the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch from 95fbae7 to a91d97c Compare June 9, 2020 16:59
jacobperron
jacobperron approved these changes Jun 9, 2020
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM

@ivanpauno
Copy link
Member Author

ivanpauno commented Jun 9, 2020 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (unrelated faliure)
  • Windows Build Status

@ivanpauno
Copy link
Member Author

It seems that this is introducing some Windows failures ....
I will try to get back to this and fix them in a near future

@ivanpauno ivanpauno force-pushed the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch from a91d97c to 2a9b1aa Compare July 1, 2020 15:03
@ivanpauno
Copy link
Member Author

ivanpauno commented Jul 1, 2020 •
edited
Loading

After yet another indexing error that was causing the windows failure (9df0fba), I reverted the commits reversing the indexing and applied the original idea of comparing with MAX size_t.
It might look a bit awkward, but it's pretty trivial to review and it fixes the signed/unsigned conversion problem, which is the goal of this PR.

@ivanpauno

This comment has been minimized.

Sign in to view
@ivanpauno
Copy link
Member Author

@ros-pull-request-builder retest this please

@ivanpauno
Copy link
Member Author

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

@ivanpauno ivanpauno force-pushed the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch from 2b5c962 to a2118b9 Compare July 3, 2020 18:50
@ivanpauno
Copy link
Member Author

  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member Author

  • Windows Build Status

@ivanpauno
Copy link
Member Author

Windows is still unhappy.
I will try again when I have the time to test what's happening in a Windows VM.

@ivanpauno ivanpauno marked this pull request as draft July 6, 2020 18:45
@ivanpauno ivanpauno force-pushed the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch from 5045b72 to 4ead91b Compare August 25, 2020 17:24
@ivanpauno
Activate usual compiler warnings and fix errors
d9d0c3a 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno force-pushed the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch from 4ead91b to d9d0c3a Compare September 14, 2020 19:28
@ivanpauno
Undor changes in tf2
70c6249 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno mentioned this pull request Sep 15, 2020
Merged
@ivanpauno ivanpauno marked this pull request as ready for review September 15, 2020 18:09
@ivanpauno
Copy link
Member Author

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

clalancette
clalancette requested changes Sep 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.

One thing to fix (and green CI), then I'm happy with this.

test_tf2/CMakeLists.txt Outdated Show resolved Hide resolved
@ivanpauno @clalancette
Use warnings only with Clang or GCC
436c98f 
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
clalancette approved these changes Sep 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, thanks for iterating @ivanpauno !

@ivanpauno
Copy link
Member Author

One extra pass:

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

@ivanpauno ivanpauno merged commit 04198d1 into ros2 Sep 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the ivanpauno/activate-usual-compiler-warnings-and-fix-errors branch September 16, 2020 16:39
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

@ahcorde ahcorde ahcorde approved these changes

@hidmic hidmic hidmic approved these changes

@jacobperron jacobperron Awaiting requested review from jacobperron

Assignees

@ivanpauno ivanpauno

Labels
bug Something isn't working in review Waiting for review (Kanban column)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ivanpauno @ahcorde @clalancette @jacobperron @hidmic