Ensure compliant node construction/destruction API. #439
Conversation
|
@clalancette @ivanpauno PTAL 99d15fb. I had to change the implementation to carry through with the node finalization despite runtime errors. It seemed like it was attempting one of those schemes of successive partial destructions. |
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>
hidmic
force-pushed
the
hidmic/ensure-compliant-node-construction-destruction
branch
from
4df46fe
to
555e7ab
Compare
|
Running CI up to
|
rmw_connext_shared_cpp/src/node.cpp
Outdated
| @@ -34,13 +37,42 @@ create_node( | |||
| const char * name, | |||
| const char * namespace_) | |||
| { | |||
| RCUTILS_CHECK_ARGUMENT_FOR_NULL(context, NULL); | |||
| assert(implementation_identifier != NULL); | |||
There was a problem hiding this comment.
This one seems like one where we should RMW_CHECK_ARGUMENT_FOR_NULL. At the very least, that will make it operate the same as the checks for other NULL below, like context.
Also, here and elsewhere, we can use nullptr instead of NULL.
There was a problem hiding this comment.
This one seems like one where we should RMW_CHECK_ARGUMENT_FOR_NULL. At the very least, that will make it operate the same as the checks for other NULL below, like context.
We could, but given that this is a somewhat internal API (not a standalone RMW implementation) I'd rather not. But I give you that an assert here is unwarranted and almost redundant with the check below. Removed it.
rmw_connext_shared_cpp/src/node.cpp
Outdated
| RMW_SET_ERROR_MSG("node handle is null"); | ||
| return RMW_RET_ERROR; | ||
| } | ||
| assert(implementation_identifier != NULL); |
There was a problem hiding this comment.
Same comment about checking and returning on NULL, and using nullptr instead.
There was a problem hiding this comment.
I thought about it, but this rmw_connext* packages are already inconsistent in this regard pretty much everywhere. If you agree, I'd prefer doing that in a separate PR.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
delete-merged-branch
bot
deleted the
hidmic/ensure-compliant-node-construction-destruction
branch
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#249.