Add a utility for rigorously initializing a message instance

[mxgrey]

This implements the feature request made here.

I still have to write tests, which I assume should go in here. I wanted to open this PR right away in case anyone has an objection to the implementation.

One thing I'd appreciate feedback on is: Should the implementations of the field initializer structs go into a separate implementation header to avoid bloating the __struct.hpp header? We could declare the field initializer structs as nested classes while putting their definitions in a separate header that automatically gets included. I think it's mostly a question of aesthetics, although splitting out the header will add some non-trivial complexity to the header generation.

[mxgrey]

Tests are added, so I'd say this is ready for review.

My one remaining concern is how many lines of code the initialization structs take up in the class definition. That could be flattened out significantly if we want to define a macro in a commonly shared header. Otherwise if we're not concerned about how many lines of code are being consumed, then this PR is good to go.

[wjwwood]

lgtm

If only we had meta-classes... https://www.fluentcpp.com/2017/08/04/metaclasses-cpp-summary/

[dirk-thomas]

Before merging this I would be curious if we can move this into a separate header - simply to keep the *__struct.hpp header readable if users take a peek. Would you be interested in trying that direction? If not, I can try to take a look at it.

[mxgrey]

I agree with wanting to make the header more readable. I can see two directions we can take:

1. Leave the Init__ struct declarations nested in the class and then put the definitions of those classes in another header. This would add complexity to the file generation procedure, and I'd have to get very creative to figure out how to make this work reliably.

2. We can add some macros to rclcpp_generator_cpp/message_initialization.hpp which take care of implementing all these details, so the actual generated header file would just look something like:

  ROSIDL_GENERATOR_CPP_INIT_FIRST_FIELD(MessageName, first_field_name);
  ROSIDL_GENERATOR_CPP_INIT_MID_FIELD(MessageName, second_field_name);
  ROSIDL_GENERATOR_CPP_INIT_MID_FIELD(MessageName, third_field_name);
  /* ... */
  ROSIDL_GENERATOR_CPP_INIT_LAST_FIELD(MessageName, last_field_name);

And there'd be a fourth macro called ROSIDL_GENERATOR_CPP_INIT_ONLY_FIELD(MessageName, only_field_name) to handle the case where a message only has one field.

Both options would occupy the same number of lines of code inside the class definition. Option (1) would add significant complexity to the file generation but it would scream at you less inside the class definition. Option (2) screams at you a bit in the class definition, but it would require absolutely no changes to the file generation scheme.

I'm more inclined towards option (2). It'll be very easy to draft, so I'll make those changes really quick, and if anyone objects to it we can revert afterwards.

[mxgrey]

The generated message headers should be much smaller now. Here's the new snippet from multi_nested__struct.hpp:

  // Initializers
  ROSIDL_GENERATOR_CPP__INIT_LAST_FIELD(MultiNested_, unbounded_sequence_of_unbounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, unbounded_sequence_of_bounded_sequences, unbounded_sequence_of_unbounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, unbounded_sequence_of_arrays, unbounded_sequence_of_bounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, bounded_sequence_of_unbounded_sequences, unbounded_sequence_of_arrays);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, bounded_sequence_of_bounded_sequences, bounded_sequence_of_unbounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, bounded_sequence_of_arrays, bounded_sequence_of_bounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, array_of_unbounded_sequences, bounded_sequence_of_arrays);
  ROSIDL_GENERATOR_CPP__INIT_MID_FIELD(MultiNested_, array_of_bounded_sequences, array_of_unbounded_sequences);
  ROSIDL_GENERATOR_CPP__INIT_FIRST_FIELD(MultiNested_, array_of_arrays, array_of_bounded_sequences);

  // Build an instance of this message in a way that requires you to explicitly
  // specify a value for every field. The compiler will help you guarantee that
  // every field is initialized.
  static Init__array_of_arrays build()
  {
    return Init__array_of_arrays();
  }

It's still a bit noisy, but I think it's roughly the same noise level as the rest of the generated header.

[mxgrey]

Is there anything left to do for this PR? Does anyone have an opinion about the new macro approach?

[dirk-thomas]

I still want to explore an alternative approach which is implemented in a separate header without having to inject all the macros into the struct header.

[mxgrey]

I think that would be undesirable because we'd be polluting the message namespace with lots of class names, since every field of every message requires its own class. Users of auto-complete (like myself) would see their auto-complete hammered with something like

Init__MessageName1__field_name_1
Init__MessageName1__field_name_2
/* ... */
Init__MessageName1__field_name_N
Init__MessageName2__field_name_1
/* ... */
Init__MessageNameM__field_name_N

We could hide that by putting those all into some kind of detail namespace, but I think making them nested classes is a more elegant approach. Also, declaring them outside of the class definition means we can't have a static build() member function in the class definition unless we forward declare these initializer classes and then define them later, which means we're potentially adding two new headers instead of one.

Maybe there's a creative solution that I'm not thinking of, but I'm very skeptical that we'll get something cleaner than this macro approach.

[mxgrey]

I also think it's worth pointing out that these macros take up fewer lines of code in the header than even the field type aliases do.

[dirk-thomas]

See 656165b which moves the builder functionality into a completely separate header. At the moment you have to #include the header with the _builder.hpp suffix explicitly. It could of course be included by the main header.

The test is using the function pkgname::msg::build_MsgName() to start the construction. I was working on an alternative templated function to enable a generic invocation rosidl_generator_cpp::build<pkgname::msg::MgsName>() but ran out of time for today...

[mxgrey]

I can understand wanting this separation, but I really think the previous API had better ergonomics:

• The need to include a whole new header seems like an unnecessary disadvantage, effectively hiding a useful feature. If we're going to keep this header separation, I would prefer to at least have it included in the API header of the message.

• It seems more intuitive to me to access the builder feature as a static function from the message type that's being built, not as a completely separate class. I feel that being its own class implies that the class is useful in some way on its own, whereas being nested in the message class makes it more clear that it's only a compilation trick to get you a message instance.

• Out of all the auto-generated gibberish in the class definition of a message, a sequence of N lines of macros (where N is the number of message fields) seems pretty innocuous to me.

I guess these details are ultimately about personal preference and aesthetics, which are notoriously difficult things to find a consensus on. So if others think this separation is aesthetically/ergonomically preferable, then I suppose it's fine with me.

But I would definitely advise against making a rclcpp_generator::build<T> template. That was my original intention for how to implement this, but then I realized we'd be forcing users to add rclcpp_generator all over the place with no benefit at all. Implementing it as a template just adds noise without accomplishing anything for anyone.

[dirk-thomas]

The need to include a whole new header seems like an unnecessary disadvantage, effectively hiding a useful feature. If we're going to keep this header separation, I would prefer to at least have it included in the API header of the message.

As mentioned above: "At the moment you have to #include the header with the _builder.hpp suffix explicitly. It could of course be included by the main header."

It seems more intuitive to me to access the builder feature as a static function from the message type that's being built, not as a completely separate class. I feel that being its own class implies that the class is useful in some way on its own, whereas being nested in the message class makes it more clear that it's only a compilation trick to get you a message instance.

There is no "completely separate class". I propose either a global function named after the data class or a templated function.

The same approach is already being applied for e.g. the function. They are being defined in a separate header to keep each file focused on one thing. It also allows to use the struct without any of the additional parts (builder, functions, whatever else) to keep it as light weight as possible. So if someone doesn't want to use the builder there is no need to test / certify that code.

Out of all the auto-generated gibberish in the class definition of a message, a sequence of N lines of macros (where N is the number of message fields) seems pretty innocuous to me.

The fact that a file is already long sounds like a bad rational for justifying even more to it.

[jacobperron]

At the moment you have to #include the header with the _builder.hpp suffix explicitly. It could of course be included by the main header.

+1 for including *__builder.hpp in the main header.

I propose either a global function named after the data class or a templated function.

I don't care if it's a global function or a method on the class, but I do think the shed would look nicer having the message namespace leading up to the builder, e.g.

geometry_msgs::msg::build_Point()
// or
geometry_msgs::msg::build::Point()
// or
geometry_msgs::msg::Point::build()

instead of:

rosidl_generator_cpp::build<geometry_msgs::Point>()

[dirk-thomas]

geometry_msgs::msg::Point::build::Point()

geometry_msgs::msg::Point is already the type of the struct.

[jacobperron]

@dirk-thomas I've edited my comment, I didn't meant to have the extra struct name in the namespace.

[dirk-thomas]

geometry_msgs::msg::build_Point()

That is the proposed API in the latest commit from me.

geometry_msgs::msg::build::Point()

This would be possible. The Naming of Point() wouldn't exactly follow the naming style but I could see to make an exception for this. If that is preferred by multiple people I can update the patch.

geometry_msgs::msg::Point::build()

Afaik this is only possible when being defined in the struct (which as mentioned above for separation of concern / avoiding optional API to become mandatory I would rather like to not do).

rosidl_generator_cpp::build<geometry_msgs::Point>()

I am happy to not follow up on this and try to make it work if it is undesired anyway.

[mxgrey]

I think if I were to choose between geometry_msgs::msg::build_Point() and geometry_msgs::msg::build::Point() I would prefer the latter (build::Point()). That helps keep the geometry_msgs::msg namespace less "polluted" (according to my own subjective ideas of symbol pollution).

[mxgrey]

Afaik this is only possible when being defined in the struct (which as mentioned above for separation of concern / avoiding optional API to become mandatory I would rather like to not do).

To be super pedantic, it only requires that the function is declared in the struct (and that the initial return type is forward declared somewhere). It could be declared inside the struct and then defined in a separate header. But then that leads to confusion if people try to use it without having all the necessary headers included, so I'd say that's probably the worst combination of design choices.

[hidmic]

+1 to geometry_msgs::msg::build::Point

[hidmic]

I'd think that builders should be templated by allocator type just like interface structs are (see here).

[mxgrey]

At the risk of beating a dead horse, we would get this trivially if we used the earlier static member function approach.

[dirk-thomas]

Since none of the ROS 2 API works at the moment with custom allocators I would rather not make this more complicated and just leave custom allocators out of this.

[hidmic]

Hmm I do see rclcpp::Publisher propagating the given AllocatorT accordingly. I don't see any MessageT allocator being used though. So I'm fine with not doing anything else on this PR, but a follow-up issue to track the missing (or half-implemented or to be deprecated) feature would be nice.

[dirk-thomas]

only requires that the function is declared in the struct (and that the initial return type is forward declared somewhere).

That is not possible since the return type is not a pointer / reference but a value so it can't be forward declared.

[dirk-thomas]

We discussed the possible option in the ROS meeting:

• geometry_msgs::msg::build_Point() disliked because of polluting the namespace and the mixed naming.
• geometry_msgs::msg::build::Point() disliked because of he ambiguity for search&replace or when using using namespace.
• geometry_msgs::msg::Point::build() requires definition of returned structs in the same header (or at least in an included header), disliked because of lower modularity.
• geometry_msgs::build<geometry_msgs::msg::Point>() preferred option, the namespace should be the package name rather than something generic like rosidl_generator_cpp used in the previous proposal. See 2a3da85 which actually implements this option.

[jacobperron]

Besides @hidmic's comment, LGTM

[dirk-thomas]

CI builds up to demo_nodes_cpp:

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

Overall LGTM but for a few comments and pending green CI

[dirk-thomas]

... and pending green CI

Above CI is green (considering the yellow builds with unrelated test failures).

[dirk-thomas]

I added special handling for EMPTY_STRUCTURE_REQUIRED_MEMBER_NAME in 1e0e96e so that the builder approach doesn't have to know about that member.

[dirk-thomas]

Minimal CI to check latest commit: