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

Fix ternary null check found by clang static analysis #176

Merged
merged 1 commit into from
Nov 30, 2020

Conversation

brawner
Copy link

@brawner brawner commented Nov 19, 2020

This null check logic is backwards. This was easily found by scan-build from clang static analysis. After this PR, class_loader builds cleanly with scan-build.

Signed-off-by: Stephen Brawner brawner@gmail.com

Signed-off-by: Stephen Brawner <brawner@gmail.com>
@brawner
Copy link
Author

brawner commented Nov 19, 2020

This PR will need to be backported to relevant branches.

Copy link

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

Ah, we never really noticed because it is in a debug print. But it is a good thing to fix. Looks good with green CI.

@brawner brawner force-pushed the brawner/fix-ternary-null-check branch 2 times, most recently from 74fc1f2 to 7a2335a Compare November 20, 2020 23:53
@brawner
Copy link
Author

brawner commented Nov 23, 2020

Running on just ci.ros2.org for now, I'll have to rebase and merge this commit as it includes some fixes for travis/appveyor. Testing --packages-select class_loader.

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

@brawner brawner force-pushed the brawner/fix-ternary-null-check branch 2 times, most recently from 02e9a39 to c4d8ac1 Compare November 23, 2020 22:35
@brawner
Copy link
Author

brawner commented Nov 23, 2020

@clalancette I don't think the travis and appveyor jobs are gonig to work with just simple updates to travis/appveyor configs. The Rpr job succeeds, and the CI jobs succeeded. The windows job shows some warnings about deprecations of cmake versions which we'll need to address elsewhere and are unrelated to this PR.

I dont' have merge privlegeges, are you able to merge it? I removed the commit for travis/appveyor updates so this can be squash merged

@clalancette
Copy link

I'll go ahead and merge it. @brawner Can you open an issue to look into fixing the Travis and Appveyor jobs? It looks like they've been consistently failing since June.

@clalancette clalancette merged commit a62623e into ros:ros2 Nov 30, 2020
@brawner
Copy link
Author

brawner commented Nov 30, 2020

See #164

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

2 participants