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.

Sign up for GitHub

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

Jump to bottom

Makes rcl_action_get_*_name() functions check for empty action names. #329

Merged
merged 2 commits into from
Nov 16, 2018
Merged

Makes rcl_action_get_*_name() functions check for empty action names. #329

merged 2 commits into from
Nov 16, 2018

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Nov 14, 2018

Connected to #306. Tiny PR to check for empty action names upon rcl_action_get_*_name() calls.

Reported by in #323 (comment).

@hidmic hidmic added the bug Something isn't working label Nov 14, 2018
@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Nov 14, 2018
@hidmic hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 14, 2018
sloretz
sloretz reviewed Nov 14, 2018
View reviewed changes
rcl_action/src/rcl_action/names.c Outdated
@@ -51,6 +57,11 @@ rcl_action_get_cancel_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate of line above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, fixed in 276aed1.

jacobperron
jacobperron reviewed Nov 14, 2018
View reviewed changes
rcl_action/src/rcl_action/names.c Outdated
@@ -30,6 +32,10 @@ rcl_action_get_goal_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_INVALID_ARGUMENT;
Copy link
Member

Choose a reason for hiding this comment

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

Suggest returning RCL_RET_ACTION_NAME_INVALID. Then it makes it possible for the action client/server to know if the provided name is empty (invalid) vs a null argument. Same for other functions and also update the docs to include the new return type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, makes sense. Done in 276aed1.

jacobperron
jacobperron reviewed Nov 14, 2018
View reviewed changes
rcl_action/src/rcl_action/names.c Outdated
@@ -51,6 +57,11 @@ rcl_action_get_cancel_service_name(
{
RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT);
Copy link
Member

Choose a reason for hiding this comment

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

Redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, yeah, fixed in 276aed1.

jacobperron
jacobperron approved these changes Nov 14, 2018
View reviewed changes
Copy link
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

@apojomovsky
Copy link
Contributor

CI status: Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Nov 15, 2018

@apojomovsky could you add CI for Windows and Mac as well ?

@apojomovsky
Copy link
Contributor

apojomovsky commented Nov 15, 2018 •
edited
Loading

@apojomovsky could you add CI for Windows and Mac as well ?

Sure!

linux-aarch64: Build Status
osx: Build Status
windows: Build Status

@mjcarroll
Copy link
Member

Re-triggering Windows: Build Status

@hidmic
Copy link
Contributor Author

hidmic commented Nov 16, 2018

Looks like MSVC complaints come from urdfdom and rviz :/

@clalancette
Copy link
Contributor

The urdfdom problems should be fixed by ros/urdfdom_headers#48 and the new release (1.0.2). Sorry for the noise.

@jacobperron
Copy link
Member

@hidmic Since the complaints are not related to this change, I think it is okay to merge.

@hidmic hidmic merged commit e59c146 into master Nov 16, 2018
@tfoote tfoote removed the in review Waiting for review (Kanban column) label Nov 16, 2018
nburek pushed a commit to nburek/rcl that referenced this pull request Nov 26, 2018
@hidmic @nburek
[rcl action] Addresses peer review comments (ros2#329)
d487533
nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018
@nburek
Revert "[rcl action] Addresses peer review comments (ros2#329)"
5eba34d 
This reverts commit d487533.
nburek added a commit to nburek/rcl that referenced this pull request Nov 26, 2018
@nburek
Revert "[rcl action] Addresses peer review comments (ros2#329)"
d4bc684 
This reverts commit d487533.
@dirk-thomas dirk-thomas deleted the hidmic/fix-empty-action-name branch April 4, 2019 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz left review comments

@jacobperron jacobperron jacobperron approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hidmic @apojomovsky @mjcarroll @clalancette @jacobperron @sloretz @tfoote