Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize namespace node and topic validation #130

Merged
merged 4 commits into from
Jan 8, 2018
Merged

Conversation

gaoethan
Copy link
Contributor

Make the result reflection more accurate, which should be
different from the default NULL
Use rmw_namespace_validation_result_string instead
for namespace validation

Signed-off-by: Ethan Gao ethan.gao@linux.intel.com

@gaoethan
Copy link
Contributor Author

#ros2/rclcpp#405

@mikaelarguedas
Copy link
Member

It looks like the string returned was changed from "X is valid" to NULL in c8d25e6.
So I'll leave it to @wjwwood to give feedback on this as I don't remember what motivated the change.

My guess is that this "matches" a return code of 0 when it succeeds, allowing you to easily know that it went well without having to do costly string operations.

If this was the motivation I would recommend returning "unknown X validation result" in the default case rather than changing the return value when the topic/node/namespace name is valid.

Good catch for the use of the wrong function in the namspace tests 👍

@clalancette
Copy link
Contributor

Doing a quick, dirty audit of the callers, it seems like almost none of them expect NULL to be returned from the validation_error_string APIs (most of them do something like RCUTILS_SET_ERROR_MSG, which will crash with a NULL error_string). Therefore, I think we should probably change it so that the validation_error_string APIs never return NULL (even in the default case), and audit all of the callers to make sure the correct thing will happen.

@gaoethan
Copy link
Contributor Author

yes, I opt to unify all the specific error string in validation_error_string to avoid extra code to deal with NULL out.

@gaoethan
Copy link
Contributor Author

@wjwwood @mikaelarguedas The NULL return is removed and unify the result sting therein, and the relevant adaption required in the components which are affected by it are referenced, please review and give your comments, thanks !

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

Thanks @gaoethan for iterating.
I'll leave @wjwwood a chance to comment on #130 (comment) before reviewing this and related PRs.
I still think that having to do string comparison instead of checking for NULL is pretty expensive and would prefer to have NULL returned for the valid cases,

@@ -47,7 +47,8 @@ TEST(test_validate_topic_name, valid_topic) {
ASSERT_EQ(RMW_RET_OK, ret);
ASSERT_EQ(RMW_TOPIC_VALID, validation_result);

ASSERT_EQ((char *)NULL, rmw_full_topic_name_validation_result_string(validation_result));
ret = strcmp("valid topic name", rmw_full_topic_name_validation_result_string(validation_result));
Copy link
Member

Choose a reason for hiding this comment

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

ret is of type rmw_ret_t that may or may not be an int in the future. I'd recommend using

ASSERT_STREQ("valid topic name", rmw_full_topic_name_validation_result_string(validation_result));

instead. (same in all tests using strcmp).

@wjwwood
Copy link
Member

wjwwood commented Nov 28, 2017

I still think that having to do string comparison instead of checking for NULL is pretty expensive and would prefer to have NULL returned for the valid cases,

Agreed. -1 to string comparison. You could instead have it return a ret code and pass a cstring pointer in to be filled with the result. But it should be possible without string comparison to know if a valid string was passed.

@gaoethan
Copy link
Contributor Author

gaoethan commented Nov 29, 2017

actually, I also think it's a little heavy to compare with the specific string, so I agree with you to return NULL for valid case. however:

  • you know, when there is NULL returned, the valid case (e.g RMW_NODE_NAME_VALID) should be checked firstly before stepping next to rmw_*_validation_result_string to make sure no NULL will returned, or it's potential to deference to NULL like in RCUTILS_SET_ERROR_MSG and rclcpp PR 405

  • If merely checking not NULL in test to avoid specific string comparing, it's not sure for really at a certain illegal case, even though it's not a big problem here for the tests, you know, non-NULL has several illegal cases.

@gaoethan
Copy link
Contributor Author

gaoethan commented Dec 13, 2017

@ros2/team, any new comments ? I can tweak it if there is to avoid its pending 😄 thanks !

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Dec 15, 2017

sorry for the delay @gaoethan I'll review this and the corresponding PRs in the next 24h 48h

Copy link
Member

@mikaelarguedas mikaelarguedas left a comment

Choose a reason for hiding this comment

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

lgtm, thanks @gaoethan

@mikaelarguedas
Copy link
Member

mikaelarguedas commented Dec 17, 2017

I just reviewed these. Once last comments are addressed we'll need to rerun CI on this set of PRs before merging

@mikaelarguedas
Copy link
Member

@gaoethan can you please rebase the branches of all these PRs? as we changed some things in the rmw API this cannot be built / tested on CI without being rebased

@gaoethan
Copy link
Contributor Author

@mikaelarguedas All have already been rebased, and you can go ahead now, :) thanks !

make the valid result more accurate, which should be
different from the default NULL
use rmw_namespace_validation_result_string instead
for namespace validation

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
and twaek the tests accordingly

Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
Signed-off-by: Ethan Gao <ethan.gao@linux.intel.com>
@mikaelarguedas
Copy link
Member

thanks @gaoethan for iterating, here's a round of ci on this set of PRs

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

@gaoethan
Copy link
Contributor Author

gaoethan commented Jan 5, 2018

@mikaelarguedas it seems that CI has passed, should it be merged now ? thanks.

@mikaelarguedas
Copy link
Member

Sorry @gaoethan for the delay, I meant to tag @wjwwood on this set of PRs to see if the change makes sense to him as he is the original author. This looks good to me and CI is still passing

I reran CI just to make sure nothing changes since.

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status (unrelated)
  • Windows Build Status (unrelated)

@wjwwood wjwwood merged commit 589c69e into ros2:master Jan 8, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jan 8, 2018
@mikaelarguedas
Copy link
Member

Thanks @wjwwood I'll merge the connected PRs as well then

@wjwwood
Copy link
Member

wjwwood commented Jan 8, 2018

Oh, sorry I missed that there were connected pr's. Thanks.

@gaoethan gaoethan deleted the bugfix branch January 9, 2018 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants