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

Ensure compliant node construction/destruction API. #206

Merged
merged 6 commits into from
Jul 15, 2020

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jul 7, 2020

Connected to ros2/rmw#249.

@hidmic hidmic requested review from eboasson and ivanpauno July 7, 2020 21:35
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

LGTM!

@hidmic hidmic requested a review from eboasson July 13, 2020 22:11
@hidmic hidmic force-pushed the hidmic/ensure-compliant-node-construction-destruction branch from 854fc84 to 2a4a8d2 Compare July 13, 2020 22:15
@hidmic
Copy link
Contributor Author

hidmic commented Jul 13, 2020

Conflicts resolved. Also adjusted changes based on discussions in ros2/rmw#249 and connected PRs.

@eboasson PTAL!

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic force-pushed the hidmic/ensure-compliant-node-construction-destruction branch from 2a4a8d2 to e2e4632 Compare July 13, 2020 22:20
@hidmic
Copy link
Contributor Author

hidmic commented Jul 13, 2020

One last rebase, I made a mistake in the previous one.

@hidmic
Copy link
Contributor Author

hidmic commented Jul 14, 2020

Running CI up to test_rmw_implementation and all RMW implementations:

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

rmw_free(const_cast<char *>(node->namespace_));
node->context->impl->fini();
rmw_node_free(node);
assert(node->context != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

As with the other PRs around this, I'll suggest that we elide this check completely. See the conversation at ros2/rmw_fastrtps#408 (comment) for more details on to why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh, I thought I had removed them all. There, see 49992cd

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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, thanks for iterating!

Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

Thanks @hidmic !

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic hidmic merged commit 368a2b5 into master Jul 15, 2020
@delete-merged-branch delete-merged-branch bot deleted the hidmic/ensure-compliant-node-construction-destruction branch July 15, 2020 13:23
ahcorde pushed a commit that referenced this pull request Oct 8, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ahcorde pushed a commit that referenced this pull request Oct 15, 2020
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
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