Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Oh oops. Maybe
rcl_node_init()
should always finalize the passed inrcl_node_options_t
even when an error occurs? At the very least it should document when it takes ownership of the options and when it doesn.tThere 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.
i actually think the
rcl_node_fini
functions also lacks the call forrcl_arguments_fini
.I wasn't sure whether the node options arguments are a real copy or not.
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.
The problem looks larger than this leak. I think adding
rcl_arguments_t
broke some assumptions aboutrcl_node_options_t
.https://github.com/ros2/rcl/blob/fdd534e19e3ee5c5551077288749053c32ddc968/rcl/src/rcl/node.c#L226-L227
rcl_arguments_t
isn't trivially copyable since it's using PIMPL, so adding it torcl_node_options_t
made that not trivially copyable as well. Maybercl_arguments_t
should become a separate parameter torcl_node_init()
.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.
Or you can make a
rcl_node_options_copy(src, dst)
function and then make sure to use it everywhere.Also, I'd express a slight preference to have who ever called
rcl_arguments_init()
(or the equivalent) also be the one to callrcl_arguments_fini()
. That is to say, havingrcl_node_init/fini
clean up the arguments is a bit undesirable because it's hard to look at this code and see that arguments are cleaned up properly. The trade-off might be that the node has to copy the given arguments structure or something.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.
On the other side, one could argue that when
rcl_node_fini
is called the complete struct including its nested types ought to be cleaned up.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.
I don't think that's on the "other side", if the clean up is done in this method, then the node would need to make a copy to store in itself, and then the node would clean up that copy in the fini function as well.
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.
I made pull request ros2/rcl#231 to make
rcl_node_init()
deep copy the options. If that's accepted then this PR could be changed to always free the arguments.I think making an
rcl_arguments_t
argument torcl_node_init()
is less error prone. Anywhere it's not passed is visible as a compiler error, while anywhere a node options structure is shallow copied is not. I didn't change that though since it would have made for a much larger PR.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.
That's a fair point, I'd also be ok with that, but the idea behind the node options is that it prevents parameter bloat in the
rcl_node_init()
function.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.
On the other hand, the default node options will never have a valid arguments field, so maybe it doesn't belong in the options struct.
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.
Even if the arguments are a separate argument to the init function, I'd say it should be copied, however.