Remove ros arguments by mjcarroll · Pull Request #454 · ros2/rclcpp

mjcarroll · 2018-03-21T18:29:59Z

Addresses rclcpp component of ros2/rcl#219

Connects to ros2/rcl#219

Additionally add init_and_remove_args for convenience.

mjcarroll · 2018-03-21T18:37:45Z

Linux
Linux-aarch64
macOS
Windows

wjwwood

lgtm overall, just some questions and suggestions

wjwwood · 2018-03-21T20:28:00Z

rclcpp/include/rclcpp/utilities.hpp

+/**
+ * Some arguments may not have been intended as ROS arguments. This function
+ * populates a the aruments in a vector. Since the first argument is always assumed
+ * to be a process name, the vector will always contain the process name.


Same style comment here, one line per sentence.

wjwwood · 2018-03-21T20:29:16Z

rclcpp/include/rclcpp/utilities.hpp

+ */
+RCLCPP_PUBLIC
+std::vector<std::string>
+init_and_remove_ros_arguments(int argc, char const * const argv[]);


We could just change the signature of init to always return the pruned list, and users could just ignore it.

wjwwood · 2018-03-21T20:34:21Z

rclcpp/src/rclcpp/utilities.cpp

+  if (rcl_parse_arguments(argc, argv, alloc, &parsed_args) != RCL_RET_OK) {
+    std::string msg = "failed to parse arguments: ";
+    msg += rcl_get_error_string_safe();
+    rcl_reset_error();


There is a function for this:

rclcpp/rclcpp/include/rclcpp/exceptions.hpp

Lines 100 to 119 in 947e3f7

/// Throw a C++ std::exception which was created based on an rcl error.

/**

* Passing nullptr for reset_error is safe and will avoid calling any function

* to reset the error.

*

* \param ret the return code for the current error state

* \param prefix string to prefix to the error if applicable (not all errors have custom messages)

* \param error_state error state to create exception from, if nullptr rcl_get_error_state is used

* \param reset_error function to be called before throwing which whill clear the error state

* \throws std::invalid_argument if ret is RCL_RET_OK

* \throws std::runtime_error if the rcl_get_error_state returns 0

* \throws RCLErrorBase some child class exception based on ret

*/

RCLCPP_PUBLIC

void

throw_from_rcl_error(

rcl_ret_t ret,

const std::string & prefix = "",

const rcl_error_state_t * error_state = nullptr,

void (*reset_error)() = rcl_reset_error);

There are other opportunities to use this below too.

wjwwood · 2018-03-21T20:40:44Z

rclcpp/src/rclcpp/utilities.cpp

+  rcl_allocator_t alloc = rcl_get_default_allocator();
+  rcl_arguments_t parsed_args;
+
+  if (rcl_parse_arguments(argc, argv, alloc, &parsed_args) != RCL_RET_OK) {


@sloretz was there going to be a function to get the already parsed arguments?

rcl_init already does this work and keeps the rcl_arguments_t around for later use. So we could use that here. The downside is that this would need rcl_init to have been called before, which might actually be a reason to leave this as-is.

Also, we'd need to ensure the argc and argv are the same ones given to the previously called rcl_init.

So with those two caveat's maybe this is the best approach.

@wjwwood You mean a function that gets the global parsed arguments? There is rcl_get_global_arguments. Both your caveats apply.

Oh yeah, forgot you ended up adding that. Any opinion on how this function should work, i.e. should it reused the result of init or do it again itself to avoid the corner cases?

As a standalone function with argc and argv passed in +1 to parsing the args again to prevent a mismatch between the arguments and the global state.

A non-public rclcpp function that made an std::vector of non-ros arguments given argc, argv, and rcl_arguments_t could be used by both remove_ros_arguments and init_and_remove_ros_arguments to avoid the double-parsing. However, inefficiency in init..() is a one-time cost in most use cases so it may not be worth the added work.

Sounds good to me too. @mjcarroll I'll leave it up to you to either use the common function or not as @sloretz described.

This issue would be OBE if we collapse init into a single signature that always returns a std::vector.

wjwwood · 2018-03-21T20:42:07Z

rclcpp/src/rclcpp/utilities.cpp

+    msg += rcl_get_error_string_safe();
+    rcl_reset_error();
+    if (NULL != nonros_argv) {
+      alloc.deallocate(nonros_argv, alloc.state);


Same question as on the other pr, I would expect this to not be required. I would expect rcl_remove_ros_arguments to clean up memory allocations in an error condition, so that it is not required here.

wjwwood · 2018-03-21T20:43:22Z

rclcpp/src/rclcpp/utilities.cpp

+  std::vector<std::string> return_arguments;
+  return_arguments.resize(nonros_argc);
+
+  for (int ii = 0; ii < nonros_argc; ++ii) {


I'd use size_t rather than int, unless you had a specific reason here.

@wjwwood if it is size_t will windows warn about signed/unsigned comparison?

That's a fair point, it might.

Also MISRA, but that's not as important here.

wjwwood · 2018-03-21T20:45:04Z

rclcpp/test/test_utilities.cpp

+  ASSERT_EQ(std::string{"process_name"}, args[0]);
+  ASSERT_EQ(std::string{"-d"}, args[1]);
+  ASSERT_EQ(std::string{"--foo=bar"}, args[2]);
+  ASSERT_EQ(std::string{"--baz"}, args[3]);


It would be good to see a test for init_and_remove_ros_arguments also or instead.

Also, I'd like to see tests for corner cases, like argc = 0 and/or argv = nullptr.

mjcarroll · 2018-03-21T23:50:54Z

Linux
Linux-aarch64
macOS
Windows

wjwwood

minor nitpick, but otherwise lgtm, I'll fix the nitpick

wjwwood · 2018-03-27T19:46:39Z

rclcpp/src/rclcpp/utilities.cpp

 // See the License for the specific language governing permissions and
 // limitations under the License.

+


minimize vertical whitespace: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace

sloretz · 2018-04-05T20:36:14Z

rclcpp/src/rclcpp/utilities.cpp

+rclcpp::remove_ros_arguments(int argc, char const * const argv[])
+{
+  rcl_allocator_t alloc = rcl_get_default_allocator();
+  rcl_arguments_t parsed_args;


Need to zero initialize this before calling rcl_get_zero_initialized_arguments(). This could be the source of the random segfault.

Possible cause of #458

Similar to the warnings when remapping to invalid namespaces, this better communicates failures to the user. Resolves ros2#449 Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Michael Carroll added 2 commits March 21, 2018 09:44

Mark arguments vector as const.

5de2473

Add C++ version of rcl_remove_ros_arguments

7a30ed0

Additionally add init_and_remove_args for convenience.

mjcarroll requested a review from wjwwood March 21, 2018 18:29

wjwwood added the in progress Actively being worked on (Kanban column) label Mar 21, 2018

wjwwood requested changes Mar 21, 2018

View reviewed changes

Address reviewer comments.

30ef8c8

wjwwood approved these changes Mar 27, 2018

View reviewed changes

minimize vertical whitespace

ff2dca4

mjcarroll merged commit ef17ec6 into master Mar 27, 2018

mjcarroll deleted the remove_ros_arguments branch March 27, 2018 21:57

mjcarroll removed the in progress Actively being worked on (Kanban column) label Mar 27, 2018

dhood mentioned this pull request Apr 2, 2018

Access violation in new test_utilities test #458

Closed

sloretz reviewed Apr 5, 2018

View reviewed changes

	/// Throw a C++ std::exception which was created based on an rcl error.
	/**
	* Passing nullptr for reset_error is safe and will avoid calling any function
	* to reset the error.
	*
	* \param ret the return code for the current error state
	* \param prefix string to prefix to the error if applicable (not all errors have custom messages)
	* \param error_state error state to create exception from, if nullptr rcl_get_error_state is used
	* \param reset_error function to be called before throwing which whill clear the error state
	* \throws std::invalid_argument if ret is RCL_RET_OK
	* \throws std::runtime_error if the rcl_get_error_state returns 0
	* \throws RCLErrorBase some child class exception based on ret
	*/
	RCLCPP_PUBLIC
	void
	throw_from_rcl_error(
	rcl_ret_t ret,
	const std::string & prefix = "",
	const rcl_error_state_t * error_state = nullptr,
	void (*reset_error)() = rcl_reset_error);

		// See the License for the specific language governing permissions and
		// limitations under the License.

Conversation

mjcarroll commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjcarroll commented Mar 21, 2018

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjcarroll commented Mar 21, 2018

Uh oh!

wjwwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mjcarroll commented Mar 21, 2018 •

edited

Loading