add rmw_validate_node_name() #93
Conversation
wjwwood
added
enhancement
wjwwood
force-pushed
the
validate_node_name_func
branch
from
4a815b7 to
468b21b
Compare
| * | ||
| * - must not be an empty string | ||
| * - must only contain alphanumeric characters and underscores (a-z|A-Z|0-9|_) | ||
| * - must not start with an number |
There was a problem hiding this comment.
@dirk-thomas I modeled these after our discussion a few days ago. Does this look ok?
There was a problem hiding this comment.
Maybe also allow dashes?
Is there a reason why not to start with a number?
There was a problem hiding this comment.
I avoided both of those for situations where the node name might be used as part of, or the start of, a variable name in some code generation situation. But I don't feel strongly about it.
wjwwood
force-pushed
the
validate_node_name_func
branch
from
468b21b to
0675ff3
Compare
wjwwood
force-pushed
the
validate_node_name_func
branch
from
0675ff3 to
3982076
Compare
| #define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3 | ||
| #define RMW_NODE_NAME_INVALID_TOO_LONG 4 | ||
|
|
||
| #define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */ |
There was a problem hiding this comment.
Also, I put an arbitrary length check of 255 here, but I excluded it from the rules in the documentation.
My thought process here is that the length limit check is a sanity check. Since there is no technical limit (that I am aware of) for node name at the moment, this is more for "did you really mean to do that?". My expectation is that "node name too long" could be just ignored by the rclcpp/rclpy crowd. I could also just remove it all together. Not sure.
There was a problem hiding this comment.
Especially for use cases with preallocated memory the upper bound is a good idea imo.
There was a problem hiding this comment.
I'll leave it then, thanks.
There was a problem hiding this comment.
+1, we can change the upper bound in the future if needed
| #define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3 | ||
| #define RMW_NODE_NAME_INVALID_TOO_LONG 4 | ||
|
|
||
| #define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */ |
There was a problem hiding this comment.
Especially for use cases with preallocated memory the upper bound is a good idea imo.
| * | ||
| * - must not be an empty string | ||
| * - must only contain alphanumeric characters and underscores (a-z|A-Z|0-9|_) | ||
| * - must not start with an number |
There was a problem hiding this comment.
Maybe also allow dashes?
Is there a reason why not to start with a number?
| * `RMW_RET_INVALID_ARGUMENT` will be returned. | ||
| * The node_name should be a valid, null-terminated C string. | ||
| * The validation_result int pointer should point to valid memory so a result | ||
| * can be stored in it as an output variable. |
There was a problem hiding this comment.
I wouldn't enforce passing this in. If the caller doesn't care about the reason for a failure it should be fine to pass 0 here. (When this is changed rmw_topic_validation_result_string should probably be updated too.)
Same for the index.
There was a problem hiding this comment.
It is required because otherwise you don't know if it is valid or not. The return code is only for exceptions, like invalid arguments.
I could make the index optional.
There was a problem hiding this comment.
I would suggest the function doesn't return RMW_RET_OK when the passed node name is invalid (e.g. empty). Maybe RMW_RET_ERROR. If the caller wants to know the reason he can optionally pass the validation_result pointer.
There was a problem hiding this comment.
@Karsten1987 and I already discussed this in #93, but I explicitly did it this way to not mix the validation codes for the node name or topic name with the new return codes. I do not want to change how it is currently organized for that reason.
rmw/src/validate_node_name.c
Outdated
| { | ||
| switch (validation_result) { | ||
| case RMW_NODE_NAME_VALID: | ||
| return "node name is valid"; |
There was a problem hiding this comment.
Do we need a result string for the successful case?
There was a problem hiding this comment.
I dunno, I suppose I could return null for valid as well.
There was a problem hiding this comment.
+1 for returning null on success
There was a problem hiding this comment.
Not my favorite solution, but I'm ok with it. See: c8d25e6
|
CI:
|
|
If anyone has time to review this I'd appreciate it. This is preventing me from moving forward on the next set of prs (#95 and beyond). |
|
One more shameless bump for a review. |
| #define RMW_NODE_NAME_INVALID_STARTS_WITH_NUMBER 3 | ||
| #define RMW_NODE_NAME_INVALID_TOO_LONG 4 | ||
|
|
||
| #define RMW_NODE_NAME_MAX_NAME_LENGTH 255 /* arbitrary constraint */ |
There was a problem hiding this comment.
+1, we can change the upper bound in the future if needed
rmw/src/validate_node_name.c
Outdated
| { | ||
| switch (validation_result) { | ||
| case RMW_NODE_NAME_VALID: | ||
| return "node name is valid"; |
There was a problem hiding this comment.
+1 for returning null on success
|
CI:
|
This is a precursor to the larger set of namespace pull requests. This one was small and reviewable as-is, so I'm submitting it separately.
The documentation for this new function also takes the opportunity to layout some rules for node names, which we've sort of had in mind informally until now.
This new function can be used by
rcl*to check if a node name follows the rules for node names, or byrmw_*packages to assert that the node name it is given is valid (should always be the case, but it might be worth checking incase something other thanrclusesrmwdirectly).Just like
rmw_validate_topic_name(), this function will return a cause if the name is invalid, as well as an index in the string that is the culprit.I copy-paste-modify'ed this from the other validate function, so please be on the lookout for typos 😄.