-
Notifications
You must be signed in to change notification settings - Fork 418
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
Remove ros arguments #454
Remove ros arguments #454
Conversation
Additionally add init_and_remove_args for convenience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm overall, just some questions and suggestions
rclcpp/include/rclcpp/utilities.hpp
Outdated
/** | ||
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same style comment here, one line per sentence.
rclcpp/include/rclcpp/utilities.hpp
Outdated
*/ | ||
RCLCPP_PUBLIC | ||
std::vector<std::string> | ||
init_and_remove_ros_arguments(int argc, char const * const argv[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just change the signature of init
to always return the pruned list, and users could just ignore it.
rclcpp/src/rclcpp/utilities.cpp
Outdated
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other opportunities to use this below too.
rclcpp/src/rclcpp/utilities.cpp
Outdated
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjwwood You mean a function that gets the global parsed arguments? There is rcl_get_global_arguments
. Both your caveats apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me too. @mjcarroll I'll leave it up to you to either use the common function or not as @sloretz described.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This issue would be OBE if we collapse init into a single signature that always returns a std::vector.
rclcpp/src/rclcpp/utilities.cpp
Outdated
msg += rcl_get_error_string_safe(); | ||
rcl_reset_error(); | ||
if (NULL != nonros_argv) { | ||
alloc.deallocate(nonros_argv, alloc.state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
std::vector<std::string> return_arguments; | ||
return_arguments.resize(nonros_argc); | ||
|
||
for (int ii = 0; ii < nonros_argc; ++ii) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use size_t
rather than int
, unless you had a specific reason here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wjwwood if it is size_t
will windows warn about signed/unsigned comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point, it might.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also MISRA, but that's not as important here.
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nitpick, but otherwise lgtm, I'll fix the nitpick
rclcpp/src/rclcpp/utilities.cpp
Outdated
@@ -12,6 +12,7 @@ | |||
// See the License for the specific language governing permissions and | |||
// limitations under the License. | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minimize vertical whitespace: https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace
rclcpp::remove_ros_arguments(int argc, char const * const argv[]) | ||
{ | ||
rcl_allocator_t alloc = rcl_get_default_allocator(); | ||
rcl_arguments_t parsed_args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to zero initialize this before calling rcl_get_zero_initialized_arguments()
. This could be the source of the random segfault.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Addresses rclcpp component of ros2/rcl#219
Connects to ros2/rcl#219