strip 'Sample_' from reported types #234
Conversation
There was a problem hiding this comment.
The question is: is it guaranteed that the service name contains Sample_? If yes, I think the conditional check should be removed and it should ensured that it is being found. If no, then the approach is fragile since a type which has the same prefix but is intentionally named like that by the user would be mangled.
| char * type_name = rcutils_strdup(type.c_str(), *allocator); | ||
| size_t n = type.find(SAMPLE_PREFIX); | ||
| std::string stripped_type = type; | ||
| if (n < type.size() - strlen(SAMPLE_PREFIX)) { |
There was a problem hiding this comment.
I would change the condition to n != std::string::npos.
| size_t n = type.find(SAMPLE_PREFIX); | ||
| std::string stripped_type = type; | ||
| if (n < type.size() - strlen(SAMPLE_PREFIX)) { | ||
| stripped_type = std::string(type.substr(0, n + 1).c_str()) + \ |
There was a problem hiding this comment.
type.substr() already returns a std::string so no need to std::string(result.c_str())` the result.
Same on the next line.
Yes I agree and share this concern. In the case of ROS services I think it will always contain that prefix.
Agreed, I didnt know how to make it less fragile at a glance that's why I used that approach. |
|
I inverted the logic in d45c98b to match the other error handling in this function. It gets rid of the I did the edit in the GitHub UI so I didn't build / lint the code - but looks fine to me 😉 |
| if (std::string::npos == n) { | ||
| RMW_SET_ERROR_MSG_ALLOC( | ||
| "failed to convert DDS type name to ROS service type name: '" | ||
| SAMPLE_PREFIX "' not found", |
There was a problem hiding this comment.
Sorry, another late note: maybe printing the actual type here would make the error message more useful (not blocking this fix though).
|
error message with b4b6cef |
| char * type_name = rcutils_strdup(type.c_str(), *allocator); | ||
| size_t n = type.find(SAMPLE_PREFIX); | ||
| if (std::string::npos == n) { | ||
| char * error_msg = rcutils_format_string( |
There was a problem hiding this comment.
The returned char * needs to be deallocated in order to not leak.
|
|
test failures not caused by this PR as they have been failing on the nightlies for a couple days ros2/build_farmer#127 (comment). Thanks @tfoote for running CI |
Opening this for visibility.
I didn't see anywhere the
Sample_prefix was defined so I ended up hard-coding it in here for now.Feedback on the approach is welcome.
Connects to ros2/ros2cli#106