Conversation
|
Ok, now that I got all the pr's open, and took a second look at it, I think these are all ready for review and to be merged after #93. |
rmw/include/rmw/rmw.h
Outdated
| RMW_WARN_UNUSED | ||
| rmw_node_t * | ||
| rmw_create_node(const char * name, size_t domain_id); | ||
| rmw_create_node(const char * name, const char * name_space, size_t domain_id); |
There was a problem hiding this comment.
Shouldn't this be one word: namespace?
There was a problem hiding this comment.
It's a keyword in c++ so when this header is included in cpp files it still fails to compile, even though this is inside an extern c block.
There was a problem hiding this comment.
That was an obvious one 🤕
I am not sure I like the idea of "separating" the variable name like this though. Some style guide recommend appending an underscore to names which collide with keywords (e.g. PEP 8). Even though this has the potential to collide with the C++ guide for class members I would prefer namespace_ over name_space in this case.
There was a problem hiding this comment.
I agree, I'll go through and update all of the prs soon.
There was a problem hiding this comment.
Ok, I think I replaced all instances of name_space with namespace_ in all the prs.
|
Can you please also create a PR for |
|
Why do I need to open a pr for opensplice? |
Because we want to continue to be able to build with |
|
I opened ros2/rmw_opensplice#164 and made this commit ros2/rmw_connext@2e65593 in order to move this all along, but honestly I think it's waste of time. I think we should rethink our policy of having opensplice and connext dynamic try to compile on a fresh clone, by disabling them in upstream with an AMENT_IGNORE, add a note to their README's telling people how to re-enable them, and have the farm remove those files when the options are selected. |
|
I rebased this after #93 was merged. |
|
Ok, with two new pr's to fix linger issues I didn't see on my mac, here is CI post rebase:
Still looking for reviews for some of these pr's. Thanks! |
|
@Karsten1987 can you review this pr too? |
|
New CI after addressing comments:
|
dirk-thomas
left a comment
There was a problem hiding this comment.
Beside the question this looks good to me.
rmw/include/rmw/validate_node_name.h
Outdated
| * - 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 | ||
| * - must start with an alphabetical character |
There was a problem hiding this comment.
Maybe I missed the discussion in another thread. Why do we want to prohibit node name starting with an underscore?
There was a problem hiding this comment.
I think I was just trying to simplify the rules, but I forgot about the underscores. The implementation didn't actually change. I'll switch it back b6f79db.
|
In 383db0f I added In 383db0f I made |
|
I believe all the comments on all pull requests have been addressed, I'm looking for final reviews. I'll probably merge later tonight if no one says anything. Here's the latest CI:
|
rmw/src/validate_namespace.c
Outdated
| break; | ||
| default: | ||
| RMW_SET_ERROR_MSG( | ||
| "rmw_validate_namespace(): unknown rmw_validate_topic_name() validation result" |
There was a problem hiding this comment.
It would be good to include the value of t_validation_result in the error message.
There was a problem hiding this comment.
I think this should work, it does on macOS at least. I'll need to run CI again to check for warning on Windows: 833c760
dirk-thomas
left a comment
There was a problem hiding this comment.
Beside the last comment this LGTM.
|
Ok, I addressed all the comments and here is passing CI:
|
|
I'm going to go ahead and merge, if there are more comments I'll address them afterwards. |
This is the first change (as it includes the actual storage for the namespace) in a series of several changes up and down the stack. The purpose of these pull requests are to add the notion of a namespace for each node. This namespace will later be used to expand relative topic names and remapping rules.
This pr builds on #93 and include the commits from there. It should be rebased after that is merged.
This particular pr is relatively simple as it only adds storage for the namespace and adds it to the API for
rmw_create_node(), see 14cd727.I'm going to leave this "in progress" until I get the other pull requests opened.