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

return NULL in case some Python function fails #240

Merged
merged 2 commits into from
Oct 16, 2017

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Oct 16, 2017

Follow up of ros2/rclpy#95.

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

Ready for review.

@dirk-thomas dirk-thomas added the in review Waiting for review (Kanban column) label Oct 16, 2017
@dirk-thomas dirk-thomas self-assigned this Oct 16, 2017
Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

LGTM, (once CI green)

I was originally curious about how these two functions were identified (in opposition to the other functions that can raise in convert_to_py). @dirk-thomas tested empirically all of them and couldn't reproduce ros2/rclpy#95 once these 2 cases were handled. So I'm in favor of merging this as is, keeping in mind that we will need to replace the remaining asserts and wrap potentially raising function calls properly in a future PR

@dirk-thomas
Copy link
Member Author

in opposition to the other functions that can raise in convert_to_py

Just for the record: they all return error codes - almost none of them raises exceptions.

@mikaelarguedas
Copy link
Member

Just for the record: they all return error codes - almost none of them raises exceptions

Yes with a few "exceptions" 😜
I agree only a handful of them can raise all the other ones are "safe"

@dirk-thomas
Copy link
Member Author

I agree only a handful of them can raise all the other ones are "safe"

The problem are not the potential exceptions here. The problem is that some functions return an error code which our code either ignores or asserts on.

@dirk-thomas dirk-thomas merged commit 22f1cd9 into master Oct 16, 2017
@dirk-thomas dirk-thomas deleted the call_object_might_return_null branch October 16, 2017 23:42
@dirk-thomas dirk-thomas removed the in review Waiting for review (Kanban column) label Oct 16, 2017
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.

2 participants