Skip to content
This repository has been archived by the owner on Oct 7, 2021. It is now read-only.

strip 'Sample_' from reported types #234

Merged
merged 5 commits into from
Jun 23, 2018
Merged

Conversation

mikaelarguedas
Copy link
Member

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

Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) + \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type.substr() already returns a std::string so no need to std::string(result.c_str())` the result.

Same on the next line.

@dirk-thomas dirk-thomas added this to the bouncy milestone Jun 22, 2018
@dirk-thomas dirk-thomas added the bug Something isn't working label Jun 22, 2018
@mikaelarguedas
Copy link
Member Author

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.

Yes I agree and share this concern. In the case of ROS services I think it will always contain that prefix.
For native ones it could be anything, but it's not really something we support ATM.

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.

Agreed, I didnt know how to make it less fragile at a glance that's why I used that approach.
I'll remove the check for now then.
As we don't allow slashes in package names of message names, it should be safe enough to use /Sample_ in the ROS case

@dirk-thomas
Copy link
Member

I inverted the logic in d45c98b to match the other error handling in this function. It gets rid of the else as well as the early defintition of stripped_type.

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",
Copy link
Member

@dirk-thomas dirk-thomas Jun 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, another late note: maybe printing the actual type here would make the error message more useful (not blocking this fix though).

@mikaelarguedas
Copy link
Member Author

error message with b4b6cef

Failed to get_service_names_and_types: failed to convert DDS type name to ROS service type name: '/Sample_' not found in type: 'myROS/ServiceType', at /home/mikael/work/ros2/bouncy_ws/src/ros2/rmw_opensplice/rmw_opensplice_cpp/src/rmw_service_names_and_types.cpp:129

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned char * needs to be deallocated in order to not leak.

@tfoote
Copy link
Contributor

tfoote commented Jun 23, 2018

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@mikaelarguedas
Copy link
Member Author

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

@mikaelarguedas mikaelarguedas merged commit ed2dd1d into master Jun 23, 2018
@mikaelarguedas mikaelarguedas deleted the fix_reported_types branch June 23, 2018 09:27
@mikaelarguedas mikaelarguedas removed the in progress Actively being worked on (Kanban column) label Jun 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants