-
Notifications
You must be signed in to change notification settings - Fork 79
Split rcl_action_bindings.cpp #1105
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
Conversation
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.
Pull Request Overview
This PR splits the existing unified rcl_action_bindings.cpp into separate files for client, goal, and server bindings and adds corresponding API functions in rcl_node_bindings.cpp. It also updates initialization in addon.cpp and the build configuration in binding.gyp to reflect the new file structure.
- Adds new action-related functions in rcl_node_bindings.cpp for fetching names and types.
- Introduces new binding files for action client, goal, and server.
- Updates addon.cpp and binding.gyp to integrate the new files.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/rcl_node_bindings.cpp | Added functions: ActionGetClientNamesAndTypesByNode, ActionGetServerNamesAndTypesByNode, and ActionGetNamesAndTypes for action API support. |
| src/rcl_action_server_bindings.h | New header defining action server bindings. |
| src/rcl_action_goal_bindings.h | New header defining action goal bindings. |
| src/rcl_action_goal_bindings.cpp | New implementation for handling action goals. |
| src/rcl_action_client_bindings.h | Renamed and updated header for action client bindings. |
| src/rcl_action_client_bindings.cpp | New implementation for handling action clients. |
| src/addon.cpp | Updated to initialize new action binding modules and removed legacy initialization. |
| binding.gyp | Updated source file list to include the new action binding files. |
Comments suppressed due to low confidence (1)
src/rcl_node_bindings.cpp:307
- [nitpick] The error message 'Failed to action server names and types' appears unclear; consider rephrasing it (e.g. 'Failed to get names and types') to better describe the failure.
THROW_ERROR_IF_NOT_EQUAL(RCL_RET_OK, rcl_action_get_names_and_types(node, &allocator, &names_and_types),
src/rcl_action_goal_bindings.cpp
Outdated
|
|
||
| *goal_handle = *rcl_action_accept_new_goal(action_server, buffer); | ||
| if (!goal_handle) { | ||
| Napi::Error::New(env, rcl_get_error_string().str) | ||
| .ThrowAsJavaScriptException(); | ||
| rcl_reset_error(); | ||
| return env.Undefined(); | ||
| } | ||
|
|
Copilot
AI
Apr 27, 2025
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.
The null check on 'goal_handle' is redundant after a successful malloc call. Consider checking the return value of rcl_action_accept_new_goal before dereferencing to properly catch failure cases.
| *goal_handle = *rcl_action_accept_new_goal(action_server, buffer); | |
| if (!goal_handle) { | |
| Napi::Error::New(env, rcl_get_error_string().str) | |
| .ThrowAsJavaScriptException(); | |
| rcl_reset_error(); | |
| return env.Undefined(); | |
| } | |
| if (!goal_handle) { | |
| Napi::Error::New(env, "Failed to allocate memory for goal handle.") | |
| .ThrowAsJavaScriptException(); | |
| return env.Undefined(); | |
| } | |
| rcl_action_goal_handle_t temp_goal_handle = | |
| rcl_action_accept_new_goal(action_server, buffer); | |
| if (!temp_goal_handle) { | |
| free(goal_handle); | |
| Napi::Error::New(env, rcl_get_error_string().str) | |
| .ThrowAsJavaScriptException(); | |
| rcl_reset_error(); | |
| return env.Undefined(); | |
| } | |
| *goal_handle = *temp_goal_handle; |
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.
Pull Request Overview
This PR splits the monolithic rcl_action_bindings.cpp file into three separate files (action client, action goal, and action server bindings) and updates the API functions in rcl_node_bindings.cpp. It also adjusts initialization in addon.cpp and the build configuration in binding.gyp to integrate the new file structure.
- New binding files: action client, goal, and server.
- Added action-related API functions in rcl_node_bindings.cpp.
- Updated addon.cpp and binding.gyp for proper integration.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/rcl_node_bindings.cpp | Added three new functions to query action client, server, and generic names/types. |
| src/rcl_action_client_bindings.* | Updated header and new file implementing client bindings with updated naming. |
| src/rcl_action_goal_bindings.* | New files providing goal binding implementations. |
| src/rcl_action_server_bindings.h | New header file for action server bindings. |
| src/addon.cpp | Integrated new action binding initialization calls. |
| binding.gyp | Updated to reference the new action binding files. |
Comments suppressed due to low confidence (3)
src/rcl_node_bindings.cpp:257
- Consider updating the error message to clearly indicate the operation, e.g. 'Failed to retrieve action client names and types.'
rcl_action_get_client_names_and_types_by_node( ... ), "Failed to action client names and types.");
src/rcl_node_bindings.cpp:285
- Consider revising the error message to 'Failed to retrieve action server names and types' for improved clarity.
rcl_action_get_server_names_and_types_by_node( ... ), "Failed to action server names and types");
src/rcl_node_bindings.cpp:309
- Update the error message to reflect the intended functionality, e.g. 'Failed to retrieve action names and types.'
rcl_action_get_names_and_types(node, &allocator, &names_and_types), "Failed to action server names and types");
This PR splits the existing rcl_action_bindings.cpp into separate files for client, goal, and server bindings and adds corresponding API functions in rcl_node_bindings.cpp. It also updates initialization in addon.cpp and the build configuration in binding.gyp to reflect the new file structure. - Adds new action-related functions in rcl_node_bindings.cpp for fetching names and types. - Introduces new binding files for action client, goal, and server. - Updates addon.cpp and binding.gyp to integrate the new files. Fix: #1104
This PR splits the existing rcl_action_bindings.cpp into separate files for client, goal, and server bindings and adds corresponding API functions in rcl_node_bindings.cpp. It also updates initialization in addon.cpp and the build configuration in binding.gyp to reflect the new file structure.
Fix: #1104