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

Use Py_XDECREF for pynode_names_and_namespaces #673

Merged
merged 1 commit into from
Jan 28, 2021

Conversation

clalancette
Copy link
Contributor

As pointed out by clang static analysis, it is possible for
pynode_names_and_namespaces to be NULL during cleanup. Thus
use Py_XDECREF everywhere to do that checking for us.

Signed-off-by: Chris Lalancette clalancette@openrobotics.org

As pointed out by clang static analysis, it is possible for
pynode_names_and_namespaces to be NULL during cleanup.  Thus
use Py_XDECREF everywhere to do that checking for us.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette
Copy link
Contributor Author

clalancette commented Jan 27, 2021

CI:

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

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Nice - is there a clang static analysis build going on on the buildfarm for this?

@clalancette
Copy link
Contributor Author

Nice - is there a clang static analysis build going on on the buildfarm for this?

Not currently, no. I've been running this locally and just fixing the bugs as I see them. The goal is indeed to have jobs available to do this either on https://ci.ros2.org, or as part of the PR jobs.

@clalancette
Copy link
Contributor Author

The test failures on macOS are the same as in the nightlies: https://ci.ros2.org/view/nightly/job/nightly_osx_repeated/2233/#showFailuresLink . So I'm going to go ahead and merge this. Thanks for the review.

@clalancette clalancette merged commit 0096a96 into master Jan 28, 2021
@clalancette clalancette deleted the clalancette/xdecref-on-cleanup branch January 28, 2021 14:05
@j-rivero
Copy link

@clalancette
Copy link
Contributor Author

Might be related to aarch64 regressions today: https://ci.ros2.org/job/nightly_linux-aarch64_repeated/1488/testReport/junit/(root)/projectroot/test_client/ ?

I don't think so; I just merged it this morning, so it wouldn't have been used in the nightlies.

@j-rivero
Copy link

I don't think so; I just merged it this morning, so it wouldn't have been used in the nightlies.

Sorry, I meant that the problem in the nightly might have been resolved by this or do you think that the root cause is different.

@clalancette
Copy link
Contributor Author

Sorry, I meant that the problem in the nightly might have been resolved by this or do you think that the root cause is different.

I think it is probably different; this only would be a problem during unlikely error paths. I'll suggest to open a new issue with that failure, since it is likely something different.

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.

3 participants