Skip to content

Conversation

@minggangw
Copy link
Member

@minggangw minggangw commented Dec 4, 2025

This PR enhances ROS argument validation by leveraging the RCL API function rcl_arguments_get_unparsed_ros() to detect and report unknown ROS arguments. The implementation replaces the simple boolean check HasUnparsedROSArgs() with a more comprehensive ThrowIfUnparsedROSArgs() function that provides detailed error messages listing the specific unknown arguments.

Key changes:

  • Implemented ThrowIfUnparsedROSArgs() to detect and report unparsed ROS arguments with detailed error messages
  • Added test coverage for unknown ROS argument detection during both context initialization and node creation
  • Integrated the new validation into both rcl_init() and rcl_node_init() code paths

Fix: #1330

Copilot AI review requested due to automatic review settings December 4, 2025 08:43
Copilot finished reviewing on behalf of minggangw December 4, 2025 08:45
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enhances ROS argument validation by leveraging the RCL API function rcl_arguments_get_unparsed_ros() to detect and report unknown ROS arguments. The implementation replaces the simple boolean check HasUnparsedROSArgs() with a more comprehensive ThrowIfUnparsedROSArgs() function that provides detailed error messages listing the specific unknown arguments.

Key changes:

  • Implemented ThrowIfUnparsedROSArgs() to detect and report unparsed ROS arguments with detailed error messages
  • Added test coverage for unknown ROS argument detection during both context initialization and node creation
  • Integrated the new validation into both rcl_init() and rcl_node_init() code paths

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/test-unparsed-args.js Adds comprehensive test coverage for unknown ROS argument detection in both context initialization and node creation scenarios
src/rcl_utilities.h Updates function signature from HasUnparsedROSArgs() to ThrowIfUnparsedROSArgs() with additional parameters for error reporting
src/rcl_utilities.cpp Implements the new function with detailed error messages and proper memory management for unparsed argument indices
src/rcl_node_bindings.cpp Integrates the new validation function into node creation with proper error handling
src/rcl_context_bindings.cpp Integrates the new validation function into context initialization (contains a critical memory leak bug)
Comments suppressed due to low confidence (1)

src/rcl_context_bindings.cpp:85

  • Memory leak: argv is allocated at line 56 but if ThrowIfUnparsedROSArgs throws an exception at line 76-78, the function returns early before registering the FreeArgs cleanup at line 85. The RCPPUTILS_SCOPE_EXIT for FreeArgs should be moved immediately after AbstractArgsFromNapiArray (after line 56) to ensure proper cleanup in all error paths, similar to how it's done in rcl_node_bindings.cpp at line 191.
  ThrowIfUnparsedROSArgs(env, jsArgv, context->global_arguments);
  if (env.IsExceptionPending()) {
    return env.Undefined();
  }

  THROW_ERROR_IF_NOT_EQUAL(
      RCL_RET_OK, rcl_logging_configure(&context->global_arguments, &allocator),
      rcl_get_error_string().str);

  RCPPUTILS_SCOPE_EXIT({ FreeArgs(argv, argc); });

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@coveralls
Copy link

coveralls commented Dec 4, 2025

Coverage Status

coverage: 82.728% (-0.02%) from 82.751%
when pulling 81e675b on minggangw:fix-1330-9
into faaa2d4 on RobotWebTools:develop.

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.

Missing rcl functions in rclnodejs

2 participants