Finalizing a zero initialized context is not allowed, contrary to the semantics of all the others rcl_*_fini functions

[ivanpauno]

Feature request

Feature description

For most rcl objects, the following code works:

rcl_<type>_t foo = rcl_get_zero_initialized_<type>();
assert(RCL_RET_OK == rcl_<type>_fini(&foo));

That's not the case for <type> = context, in that case it's returning RCL_RET_INVALID_ARGUMENT,

e.g.:

• rcl_publisher_fini.
• rcl_subscription_fini.
• rcl_context_fini.

I ran into this while working in rcljava.
Though this might be considered a bug in rcljava code, it's strange that we follow different semantics in constructor/destructor functions of different objects in the same package.

I think that it would also be great to have a clear style guide on how to name constructor/destructor in C code.

[hidmic]

it's strange that we follow different semantics in constructor/destructor functions of different objects in the same package.

I fully agree, but I'd argue finalization of a zero initialized object should not succeed, and code attempting to finalize any given object more than once it's not handling ownership and/or lifetimes properly.

[ivanpauno]

I fully agree, but I'd argue finalization of a zero initialized object should not succeed, and code attempting to finalize any given object more than once it's not handling ownership and/or lifetimes properly.

I don't mind one way or the other, but I would like to have standardized behavior.

The pattern of initialization we use in rcl is pretty much the same of what in cpp is called "two steps" initialization.
You have a noexcept default (or zero) constructor, an init function that may fail, and the destructor always run either you called the init function or not.
If you use that analogy, the rcl_*_fini function should handle "default" (or zero) initialized objects.

The only advantage of the "fini" function not accepting a "default" (or zero) initialized object is the following: if the "fini" function resets the object to the "default" state, you can then detect double "fini".

[sloretz]

+1 to fini being allowed to fini a zero initialized struct.

Say there is a C function that initializes and fini's a temporary object. If fini can safely fini a zero-initialized thing, then it's safe to have a common cleanup routine without tracking if a thing is already initialized.

{
  // ... other stuff might have already been created
  rcl_thing_t thing = rcl_get_zero_initialized_thing();
  if (RCL_RET_OK != rcl_thing_init(&thing)) {
    // thing might still be zero initialized here!
    goto cleanup;
  }

   // ... Other code with error cases that jump to cleanup

  // Sucessful code passes through cleanup
  cleanup:
    // ... don't care what happened above because it's safe to fini zero initialized
    rcl_thing_fini(thing);
    // ...
    return asdf;
}

It seems intuitive for a C++ user since it's safe to delete a nullptr

[cottsay]

It might be noteworthy that for variable-length types elsewhere in the codebase (like arrays, vectors, etc), we must allow a zero-initialized object to be finalized because a properly initialized struct with a size of 0 could result in a zero-initialized struct on systems where malloc(0) is allowed to return NULL under normal operation. That specific case clearly doesn't apply here, but it does set forth a precedent where we're already allowing finalization of several of our defined types despite being zero-initialized.

[clalancette]

+1 to fini being allowed to fini a zero initialized struct.

Yeah, I agree with @sloretz about this for the same reasons.

@wjwwood to give an opinion here as well.

[jacobperron]

👍 For changing the behavior of rcl_context_fini for consistency. I went ahead and proposed the change: #842