Inconsistent struct/pointer arguments for allocators

[mosteo]

I don't see a reason why some functions expect a struct and others a pointer:

Pointer: https://github.com/ros2/rcl/blob/d13a902d9ad7d5cc9f2b5bce084f823a2e66589d/rcl/include/rcl/graph.h#L86 https://github.com/ros2/rcl/blob/d13a902d9ad7d5cc9f2b5bce084f823a2e66589d/rcl/include/rcl/graph.h#L129

Struct: https://github.com/ros2/rcl/blob/d13a902d9ad7d5cc9f2b5bce084f823a2e66589d/rcl/include/rcl/graph.h#L212 https://github.com/ros2/rcl/blob/d13a902d9ad7d5cc9f2b5bce084f823a2e66589d/rcl/include/rcl/timer.h#L138

[dirk-thomas]

Thanks for reporting this inconsistency. There is no reason why some API takes a pointer and some takes a value. This should indeed be unified.

Just as a side note when choosing which one to use: if an rcl_allocator_t would ever store any data directly (rather then behind another pointer) the altering only the members of a copied value might be undesired...

[wjwwood]

Indeed it needs to be cleaned up, though I don't think we necessarily need to use the same thing everywhere. The pointer should be preferred, as @dirk-thomas pointed out, so that if the allocator struct becomes non-trivial it is still efficient. However, in cases where the only possible error is that the allocator pointer is null (e.g. a "getter" where the allocator is used only for error processing), some functions return the value directly rather than an rcl_ret_t. So by making the allocator struct pass by value, it avoids the check for a null pointer in to an allocator struct.

In the end, making everything a pointer (and reducing the number of functions that need an allocator by doing things like making the macro to set the error message no longer allocate memory) might be the way to go, but I'd advise against a blanket policy despite good reasons to do the opposite in certain cases.

[mosteo]

From my point of view as a binding implementer the main issue when passed as pointer is that, if copies are of the pointer are kept, I have to manage the lifetime on my side; whereas if it is always passed by copy the issue does not exist.

For example, just this week-end I realized (via segfaults) that rcl_clock_t indeed keeps a copy of the allocator pointer received during initialization. In the case of RCL types, which are themselves containers of pointers, I find it a bit more confusing the passing of pointers for no explicit reason.

I rarely code in C these days, so I'm a bit out of touch about if there are established conventions to use by copy/by pointer to express const/non const usage, common practices for pointer ownership, etc, so I'm sorry if I'm going over something obvious (like I should assume worst case whenever I pass a pointer around).

[wjwwood]

For example, just this week-end I realized (via segfaults) that rcl_clock_t indeed keeps a copy of the allocator pointer received during initialization.

That's probably a bug, if a function that initializes something takes a pointer to an allocator and needs to use the allocator later, then it should make a copy. The things that the allocate points to should be valid longer than the lifetime of all objects that use it (it's up to the user to ensure that).

If you do something like rcl_allocator_t * my_allocator = (...)malloc(...);, then copy some existing allocator into it (e.g. *my_allocator = rcl_get_default_allocator();), and then use that pointer to call a function like rcl_clock_init(..., my_allocator) (not sure about the names there), then rcl_clock_init should copy it, so that you can be assured that you can free(my_allocator) after calling the function.

A separate question is why you're passing a pointer to a temporary allocator (maybe it's on stack and you pass &my_allocator to get a pointer)?

[mosteo]

Indeed in this case I was passing a temporary allocator in the stack (for some minor convenience related to Ada). I was overconfident that the pointer wouldn't be copied since e.g.rcl_timer_init takes a copy so I thought it was the minor inconsistency I reported, but it turns out that rcl_clock_fini crashes. Then I saw that rcl_clock_h contains an rcl_allocator_t *, so I reworked my side to ensure the lifetime of that allocator.

This is the line that copies the pointer. I understand from your explanation about expected usage that that's a bug then?

[wjwwood]

This is the line (https://github.com/ros2/rcl/blob/d13a902d9ad7d5cc9f2b5bce084f823a2e66589d/rcl/src/rcl/time.c#L140) that copies the pointer. I understand from your explanation about expected usage that that's a bug then?

I believe so, yes.

[wjwwood]

I made an issue for the problem: https://github.com/ros2/rcl/issues/267