Skip to content

Activate usual compiler warnings and fix errors#270

Merged
ivanpauno merged 3 commits intoros2from
ivanpauno/activate-usual-compiler-warnings-and-fix-errors
Sep 16, 2020
Merged

Activate usual compiler warnings and fix errors#270
ivanpauno merged 3 commits intoros2from
ivanpauno/activate-usual-compiler-warnings-and-fix-errors

Conversation

@ivanpauno
Copy link
Copy Markdown
Member

@ivanpauno ivanpauno commented Jun 1, 2020

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
Copy Markdown
Member Author

ivanpauno commented Jun 1, 2020

@ros-pull-request-builder retest this please

hidmic
hidmic reviewed Jun 1, 2020
View reviewed changes
@ahcorde
Copy link
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Jun 2, 2020

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
Copy Markdown
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
Copy Markdown
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
@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
Copy Markdown

@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
Copy Markdown
Member Author

ivanpauno commented Jun 2, 2020

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

  • Linux Build Status

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jun 2, 2020

@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
Copy Markdown
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.

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Jun 10, 2020

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
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Jul 1, 2020

@ros-pull-request-builder retest this please

@ivanpauno ivanpauno requested review from hidmic and jacobperron July 1, 2020 18:18
@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 1, 2020

  • 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
Copy Markdown
Member Author

ivanpauno commented Jul 6, 2020

  • macOS Build Status
  • Windows Build Status

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 6, 2020

  • Windows Build Status

@ivanpauno
Copy link
Copy Markdown
Member Author

ivanpauno commented Jul 6, 2020

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
Copy Markdown
Member Author

ivanpauno commented Sep 15, 2020

  • 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
Copy Markdown
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.

@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
Copy Markdown
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
Copy Markdown
Member Author

ivanpauno commented Sep 16, 2020

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.

5 participants

@ivanpauno @ahcorde @clalancette @jacobperron @hidmic