Skip to content

rclpy_init() passes command line arguments to rcl_init() #179

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 6, 2018
Merged

rclpy_init() passes command line arguments to rcl_init() #179

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d5ede9c
rclpy_init() passes command line arguments to rcl_init()
sloretz Mar 5, 2018
023ccaf
comment that exception is already raised
sloretz Mar 6, 2018
0dc3482
Fix windows warning about data loss
sloretz Mar 6, 2018
93c634b
Pass NULL when num_args == 0
sloretz Mar 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 44 additions & 7 deletions rclpy/src/rclpy/_rclpy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,51 @@ rclpy_trigger_guard_condition(PyObject * Py_UNUSED(self), PyObject * args)
* Raises RuntimeError if rcl could not be initialized
*/
static PyObject *
rclpy_init(PyObject * Py_UNUSED(self), PyObject * Py_UNUSED(args))
rclpy_init(PyObject * Py_UNUSED(self), PyObject * args)
{
// TODO(esteve): parse args
rcl_ret_t ret = rcl_init(0, NULL, rcl_get_default_allocator());
if (ret != RCL_RET_OK) {
PyErr_Format(PyExc_RuntimeError,
"Failed to init: %s", rcl_get_error_string_safe());
rcl_reset_error();
// Expect one argument which is a list of strings
PyObject * pyargs;
if (!PyArg_ParseTuple(args, "O", &pyargs)) {
return NULL;
Copy link
Member

@wjwwood wjwwood Mar 6, 2018

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?

Copy link
Contributor Author

@sloretz sloretz Mar 6, 2018

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.

Copy link
Contributor Author

@sloretz sloretz Mar 6, 2018

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

}

pyargs = PySequence_List(pyargs);
if (NULL == pyargs) {
return NULL;
}
Py_ssize_t num_args = PyList_Size(pyargs);
Copy link
Member

@wjwwood wjwwood Mar 6, 2018

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?

Copy link
Contributor Author

@sloretz sloretz Mar 6, 2018

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.


rcl_allocator_t allocator = rcl_get_default_allocator();
char ** arg_values = allocator.allocate(sizeof(char *) * num_args, allocator.state);
if (NULL == arg_values) {
PyErr_Format(PyExc_MemoryError, "Failed to allocate space for arguments");
Py_DECREF(pyargs);
return NULL;
}

bool have_args = true;
for (int i = 0; i < num_args; ++i) {
// Returns borrowed reference, do not decref
PyObject * pyarg = PyList_GetItem(pyargs, i);
if (NULL == pyarg) {
have_args = false;
break;
}
// Borrows a pointer, do not free arg_values[i]
arg_values[i] = PyUnicode_AsUTF8(pyarg);
}
Copy link
Member

@mikaelarguedas mikaelarguedas Mar 6, 2018

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?

Copy link
Contributor Author

@sloretz sloretz Mar 6, 2018

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


if (have_args) {
rcl_ret_t ret = rcl_init(num_args, arg_values, allocator);
if (ret != RCL_RET_OK) {
PyErr_Format(PyExc_RuntimeError, "Failed to init: %s", rcl_get_error_string_safe());
rcl_reset_error();
}
}
allocator.deallocate(arg_values, allocator.state);
Py_DECREF(pyargs);

if (PyErr_Occurred()) {
return NULL;
}
Py_RETURN_NONE;
Expand Down