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

Add CLI args to Node constructor #461

Merged
merged 2 commits into from Apr 17, 2018
Merged

Add CLI args to Node constructor #461

merged 2 commits into from Apr 17, 2018

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Apr 14, 2018

This adds a parameter for command line arguments to Node, and a parameter to ignore global CLI arguments. I think roslaunch might need something like this to dynamically compose nodes without their arguments affecting other nodes, though I'm not sure if there is another strategy planned.

Kind of related to ros2/examples#200 and ros2/ros2#450

@sloretz
Add CLI args to Node constructor
3fc6aed 
Adds arguments and use_global_arguments to NodeBase
@sloretz sloretz added the in review Waiting for review (Kanban column) label Apr 14, 2018
@sloretz sloretz self-assigned this Apr 14, 2018
@sloretz sloretz requested a review from wjwwood April 14, 2018 00:28
This was referenced Apr 14, 2018
Merged
Merged
@sloretz
Copy link
Contributor Author

sloretz commented Apr 14, 2018

CI

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

sloretz
sloretz commented Apr 14, 2018
View reviewed changes
rclcpp/src/rclcpp/node_interfaces/node_base.cpp
@@ -84,18 +87,33 @@ NodeBase::NodeBase(
}

// Create the rcl node and store it in a shared_ptr with a custom destructor.
rcl_node_t * rcl_node = new rcl_node_t(rcl_get_zero_initialized_node());
std::unique_ptr<rcl_node_t> rcl_node(new rcl_node_t(rcl_get_zero_initialized_node()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this only to get rid of false positive "double free" error from cppcheck. It didn't like that it saw two delete rcl_node; statements: one in the error handling after calling rcl_parse_arguments(), and another in the error handling of rcl_node_init().

wjwwood
wjwwood approved these changes Apr 14, 2018
View reviewed changes
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm

@sloretz
Copy link
Contributor Author

sloretz commented Apr 16, 2018

checking if windows warning is fixed Build Status

@sloretz
Copy link
Contributor Author

sloretz commented Apr 16, 2018 •
edited

Remaining CI

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

@sloretz sloretz merged commit 360f1b9 into master Apr 17, 2018
@sloretz sloretz deleted the pass_cli_args_to_node branch April 17, 2018 17:52
@sloretz sloretz removed the in review Waiting for review (Kanban column) label Apr 17, 2018
@Karsten1987
Copy link
Contributor

I believe this PR introduced some memory leaks when node_name or namespace_ are incorrect.
It primarily allocates memory which doesn't get freed when one of the following checks fails and raises an exception.

I ran valgrind on test_node and the following tests leaks twice the memory of the node arguments
https://github.com/ros2/rclcpp/blob/master/rclcpp/test/test_node.cpp#L37-L53
As the first test runs correctly, no memory is leaked. However, as the second and third node result in an exception, the node argument memory is leaked.

@Karsten1987 Karsten1987 mentioned this pull request Apr 30, 2018
Closed
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this pull request Aug 5, 2022
@Karsten1987
reenable cppcheck for rosbag2_transport (ros2#461)
8e71e9c 
* reenable cppcheck

Signed-off-by: Karsten Knese <karsten@openrobotics.org>

* suppress unknown macro inline

Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wjwwood wjwwood wjwwood approved these changes

Assignees

@sloretz sloretz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sloretz @Karsten1987 @wjwwood