-
Notifications
You must be signed in to change notification settings - Fork 91
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
Ensure compliant node construction/destruction API. #206
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
854fc84
to
2a4a8d2
Compare
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>
2a4a8d2
to
e2e4632
Compare
One last rebase, I made a mistake in the previous one. |
rmw_cyclonedds_cpp/src/rmw_node.cpp
Outdated
rmw_free(const_cast<char *>(node->namespace_)); | ||
node->context->impl->fini(); | ||
rmw_node_free(node); | ||
assert(node->context != nullptr); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
There was a problem hiding this 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!
There was a problem hiding this 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>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#249.