Conversation
tfoote
left a comment
There was a problem hiding this comment.
This also follows a different paradigm where the other APIs return a pointer to an internal string versus returning a pointer to a recently allocated string and expecting the user to free it. This asymmetry in the API is likely to cause coding errors.
To that end I might suggest that the fq_name be precomputed and stored whenever the name and namespace are set, and then a pointer to that is returned by this function instead of the function allocating new memory just to return the string.
rcl/include/rcl/node.h
Outdated
| * <hr> | ||
| * Attribute | Adherence | ||
| * ------------------ | ------------- | ||
| * Allocates Memory | Yes |
There was a problem hiding this comment.
It would be great to be able to use this in applications that require allocation free operations.
There was a problem hiding this comment.
Fixed.
name and namespace are stored in the specific rmw implementation (if I understand correctly) and I hesitated from making changes across many repos. Thats why I decided to store the computed fq_name in the rcl_node_impl_t structure.
tfoote
left a comment
There was a problem hiding this comment.
Thanks that looks good now. I've triggered CI
- Linux
- Linux-aarch64
- macOS
- Windows
|
@rarvolt This looks good overall. It failed CI so I rebased it and pushed to a local branch. If you have a moment to rebase this yourself that would be great. As well we've added a DCO that would be great if you could sign off on by signing your commits. We're not yet strictly enforcing the DCO during the transition but it would be great if you'd be willing to try it out and give us any feedback at this stage. More info on the DCO is here: https://probot.github.io/apps/dco/ |
|
The rebase passed in #381 If you have a chance to rebase it before tomorrow that would be great. Otherwise I'll merge my rebase. |
|
Superceded by #381 |
This fixes #255