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 rmw_get_graph_guard_condition #32

Merged
merged 1 commit into from
Jun 1, 2016
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 1, 2016

Previously it was just returning junk, which was left over from my wait_for_service work.

This doesn't implement it, but at least it returns a valid guard condition. I wouldn't return anything at all, but rcl_node_init calls it, so I have to return something valid or else all fastrtps programs would fail.

previously it was just returning junk
@wjwwood
Copy link
Member Author

wjwwood commented Jun 1, 2016

CI:

  • Linux:
    • Build Status
  • OS X:
    • Build Status
  • Windows:
    • Build Status

@tfoote
Copy link

tfoote commented Jun 1, 2016

lgtm, there are 6 instances of the 7 lines to get the participant from the impl that could be turned into a function. If there's expected to be more instances the helper function might be worth it. Otherwise we can leave it as is.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 1, 2016

Yeah, there's a lot of improvements that could be made, but with respect to deduplicating the code, in about half the cases you have to return NULL and the other half return RMW_RET_ERROR if there is a problem. This is why we have common macros to encapsulate this in rmw:

https://github.com/ros2/rmw/blob/master/rmw/include/rmw/impl/cpp/macros.hpp

And in rcl as well:

https://github.com/ros2/rcl/blob/master/rcl/src/rcl/common.h#L26-L33

I'd leave it as-is for now and try to deduplicate code between this and the other rmw implementations in the future.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 1, 2016

CI looks good (existing warnings on Windows and flaky tests on Linux).

@wjwwood wjwwood merged commit 2fefcf4 into master Jun 1, 2016
@wjwwood wjwwood deleted the fix_graph_guard_condition branch June 1, 2016 04:53
try {
node_impl = new FastRTPSNodeImpl();
} catch(std::bad_alloc & exc) {
RMW_SET_ERROR_MSG("failed to allocate node impl struct");
Copy link
Member

@dirk-thomas dirk-thomas Jun 1, 2016

Choose a reason for hiding this comment

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

Doesn't this leak graph_guard_condition?

Same for the other return statements below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The function was not setup to handle unrolling. None of the functions in this file are except one. So I didn't want to change that in these changes. In general I think this time implementation needs some clean up.

Copy link
Member Author

Choose a reason for hiding this comment

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

#33

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