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

[osrf_testing_tools_cpp] Add warnings #54

Merged
merged 4 commits into from
Dec 1, 2020
Merged

[osrf_testing_tools_cpp] Add warnings #54

merged 4 commits into from
Dec 1, 2020

Conversation

audrow
Copy link
Collaborator

@audrow audrow commented Sep 25, 2020 •
edited
Loading

This PR enables Wformat=2, Wconversion, Woverloaded-virtual, Wshadow, and Wnon-virtual-dtor in osrf_testing_tools_cpp.

clalancette
clalancette approved these changes Sep 25, 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 to me with green CI.

@audrow
Copy link
Collaborator Author

audrow commented Sep 29, 2020

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

@audrow
Copy link
Collaborator Author

audrow commented Oct 7, 2020 •
edited
Loading

Running different gtest versions to see if we can fix the warnings:

  • gtest v1.8.0 (what is currently used; not rebuilt from above)
    • macOS Build Status
  • gtest v1.10.0
    • macOS Build Status

Edit: These CI runs are irrelevant because this package has its own copy of googletest (here), which uses v1.8.1 (ament/googletest uses v1.8.0).

@audrow
Copy link
Collaborator Author

audrow commented Oct 7, 2020

Ah, this packages seems to be using its own internal version of gtest (v1.8.1).

@audrow audrow mentioned this pull request Oct 15, 2020
Merged
@audrow
Copy link
Collaborator Author

audrow commented Nov 20, 2020

3 out of 36 warnings remain on macOS after googletest was updated. They seem reasonable. I'll try to fix them and then rerun CI.

  • macOS Build Status

@audrow
Copy link
Collaborator Author

audrow commented Nov 24, 2020

Here is another run of macOS's CI:

  • macOS Build Status

@audrow
Copy link
Collaborator Author

audrow commented Nov 24, 2020

Here's a full run of CI (I reused macOS CI run from my previous comment):

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

@audrow
Copy link
Collaborator Author

audrow commented Nov 24, 2020

#61 is for the Windows warnings.

@audrow
Add easy warnings
61555c4 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Cast argc to unsigned long
ad4d4ed 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Static cast stack trace in backtrace allocator
4404ea5 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
@audrow
Copy link
Collaborator Author

audrow commented Dec 1, 2020

I've rebased on master now that #61 has been merged. Here's a new CI run:

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

@audrow
Change argc cast to size_t
e18092d 
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
clalancette
clalancette approved these changes Dec 1, 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 to me with green CI.

@audrow
Copy link
Collaborator Author

audrow commented Dec 1, 2020

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

@audrow
Copy link
Collaborator Author

audrow commented Dec 1, 2020

@clalancette, I don't have permission to merge in this repo. Could you merge this in when you have a chance?

@clalancette clalancette merged commit d44022f into osrf:master Dec 1, 2020
@audrow audrow deleted the audrow/make-warnings-osrf-testing-tools-cpp branch December 1, 2020 23:47
@christophebedard christophebedard mentioned this pull request Apr 22, 2021
Closed
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@audrow @clalancette