-
Notifications
You must be signed in to change notification settings - Fork 238
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
rclpy_init() passes command line arguments to rcl_init() #179
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.
Other than some questions (mostly just curious), this lgtm.
rclpy/src/rclpy/_rclpy.c
Outdated
// Expect one argument which is a list of strings | ||
PyObject * pyargs; | ||
if (!PyArg_ParseTuple(args, "O", &pyargs)) { | ||
return NULL; |
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.
Just to check, these functions setup the exceptions right? That's why only returning null here is fine?
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.
Yup, PyArg_ParseTuple returns false and sets an exception on failure.
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.
Added comments about exceptions already being set in 023ccaf
rclpy/src/rclpy/_rclpy.c
Outdated
if (NULL == pyargs) { | ||
return NULL; | ||
} | ||
Py_ssize_t num_args = PyList_Size(pyargs); |
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 guess this cannot fail?
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 can raise SystemError and return -1 https://github.com/python/cpython/blob/745dc65b17b3936e3f9f4099f735f174d30c4e0c/Objects/listobject.c#L188 if PyList_Check
returns false, but I don't think that's possible if PySequence_List
above succeeds.
rclpy/src/rclpy/_rclpy.c
Outdated
} | ||
// Borrows a pointer, do not free arg_values[i] | ||
arg_values[i] = PyUnicode_AsUTF8(pyarg); | ||
} |
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.
can we just skip this entire block (L180 to 197) when num_args
is 0 and pass a NULL
pointer to rcl_init
instead?
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.
Block skipped when no args in 93c634b
CI with 93c634b |
This passes command line arguments through to rcl. Previously there was only a TODO message. This PR allows python nodes to be remapped using ros2/rcl#217.
CI
connects to ros2/ros2#450