Review of the rosidl_generator_cpp package

[dirk-thomas]

Comments / feedback

Please provide feedback in separate comments - one per topic - to allow hiding them when they have been resolved - to keep the discussion readable and pending items visible.
Beside any additional feedback it is also valuable to comment if you agree/disagree with the check boxes indicating proposed to-dos.

Scope of the package

At the moment the package has two purposes:

1. provide the message generator for C++ which generates C++ code / library at the build time of each message package
2. provide the C++ headers which messages will depend on when build in downstream packages

This poses an unnecessary runtime dependency for message packages.
Therefore this package should be split into two parts.

• This will be addressed in Splitted rosidl_generator_c and rosidl_generator_cpp in two: rosidl_generator_x and rosidl_runtime_x #443 which will separate the first part (the generator) from the second part (the runtime component).

Package dependencies

• build dependency on ament_cmake: since it is an ament_cmake package

• export the build dependencies on ament_cmake: it registers itself as an extension

  • Reducing the dependency to ament_cmake_core might be feasible?
    See only export ament_cmake_core instead of ament_cmake #459.

• export the build dependencies on rosidl_cmake: it uses the CMake API provided by that package when generating code for message packages

• export the build dependencies on rosidl_generator_c: since the headers transitively include headers from that package

All of these dependencies look reasonable.

• They will be split as part of Splitted rosidl_generator_c and rosidl_generator_cpp in two: rosidl_generator_x and rosidl_runtime_x #443 between the generator and runtime package

ℹ️ The test dependencies are not being reviewed since they are not part of the public interface of the package.

The group membership currently lists both groups rosidl_generator_packages as well as rosidl_runtime_packages.

• Those will also be split as part of Splitted rosidl_generator_c and rosidl_generator_cpp in two: rosidl_generator_x and rosidl_runtime_x #443 between the generator and runtime package

Directories and Files

bin

The script contains the CLI for the C++ generator.
All logic is in the Python module.

cmake

• register_cpp.cmake contains a macro to register the rosidl_generate_idl_interfaces extension.
• rosidl_generator_cpp_extras.cmake sets some CMake variables an invokes the above macro to register the extension.
• rosidl_generator_cpp_generate_interfaces.cmake contains the CMake logic of the C++ generator extension which generates the C++ code for interfaces and installs the headers.

include

All C++ headers end in .hpp.

All headers are in the subdirectories rosidl_generator_cpp and rosidl_typesupport_cpp.

• The headers in rosidl_typesupport_cpp declare the templated function to retrieve type support handles.
  In the C generator these symbols are in the same directory (though not templated).
  Consider unifying this to make it more similar between the two generators. See below comments why these headers are in a separate namespace.

While #443 moves the headers into the new package rosidl_runtime_c it doesn't rename the directory.
As a consequence functions and types are still prefixed with the package name rosidl_generator_cpp.

• Consider renaming the directory / prefix to match the package name after the headers have been moved.
  The trade off will be the amount of external code which needs to be updated for this.
  See rename rosidl_generator_cpp namespace to rosidl_runtime_cpp #456.

Each header is named after the class / struct it declares or contains functions related to a certain type.
Since all headers are in a subdirectory rosidl_generator_cpp / rosidl_typesupport_cpp their filename doesn't contain that part but all symbols are defined in the namespace rosidl_generator_cpp / rosidl_typesupport_cpp.

resource

The generated code the templates expand to is split into multiple files:

• <interface-name>.hpp is the header commonly included by code using the interface.
  This file includes the following two headers:

  • <interface-name>__struct.hpp contains the structs of the messages.
    For a message that is a single struct whereas for a service it is the structs for the request and response.

  • <interface-name>__traits.hpp declares carious type traits to determine specific characteristics of the type at compile time (e.g. is_message, data_type providing the type string, has_fixed_size, has_bounded_size).

Downstream packages commonly only use the generated code.
Therefore the API of the templates isn't considered public.

The templates starting with idl are the once being evaluated first.
Some of those templates then include other templates starting with action, msg, and srv depending on the interface type.

Currently the generated code doesn't include any .cpp files and therefore no library.

• The implementation of the methods could be moved into .cpp files which would require each interface package to export a library.
  Ticketed in consider moving the implementation part of generated C++ interfaces into .cpp files #462.

src

There are no source files in this package.

• The implementation of the bounded vector could be moved into a .cpp file which would require the package to export a library.

rosidl_generator_cpp

The Python module contains the logic to parse IDL files and generate C++ code for them.
User land code commonly doesn't interact with this API but only uses the CMake extension to trigger the code generator in an interface package.

test

The unit tests aren't part of the public interface of the package and are therefore not being considered here.

Naming of API

• rosidl_generator_cpp::BoundedVector: offer a similar data structure as a std::vector but with an upper bound specified as a template argument.

• All type traits are defined in the namespace rosidl_generator_traits.

  • Moving these into the rosidl_generator_cpp namespace seems more consistent and clarifies where they are being defined when being used.

• rosidl_generator_cpp::MessageInitialization: defines different initialization options if/how the memory of an interface struct should be initialized with.
  The enum value are based on the values defined in the rosidl_generator_c package.

API signatures

Messages

Each interface struct can be templated with an allocator.
The template has a trailing underscore.

A typedef with the name of the interface is being provided for convenience which uses a default allocator.

Each interface struct contains the field names as member variables - without any prefix / suffix.

The constructor optionally takes an enum value to choose the initialization strategy.
The caller can choose if fields should:

• not be initialized (fastest),
• initialized with zeros,
• only initialize fields with a default or
• initialize all fields with default values if applicable or zeros (default).

Services

Beside two messages for the request and response part a service struct if defined which only contains these two types: Request and Response.

Actions

Beside user specified type for the goal, result and feedback an action struct if defined which contains these three types (Goal, Result, and Feedback) as well and internal Impl struct with the derived types wrapping the user specific types (SendGoalService, GetResultService, and FeedbackMessage) and providing common interfaces (CancelGoalService and GoalStatusMessage).

[mxgrey]

In the design doc it's explained that there are four message initialization options to choose from, where all of them are either initializing the message fields to some kind of default or else not initializing the field(s) at all.

The rationale for not providing a constructor that takes in positional arguments to initialize the field members is because the nature of C++ having unnamed function arguments has the risk of introducing subtle bugs when message definitions get changed. This is true, but it doesn't address the bugs that are likely to happen when a developer forgets to initialize a certain message field in cases where the default value is unacceptable (... I just spent an embarrassing few days debugging this exact issue after refactoring a message definition and forgetting some of the places where that message was getting used).

To deal with both of these edge cases, I'd like to propose what I'm dubbing an "Opinionated Builder Pattern": a twist on the classic builder pattern that provides a compile-time guarantee that every field of a message will be explicitly initialized by you, by the name of that field, before you are able to hold the message yourself. This would be an optional way to construct a message, while the existing constructors remain exactly as they are.

A proof of concept to illustrate the idea can be found here.

You will only be required to explicitly define every single field if you choose to construct the message using the build<T>() method. The normal message constructors will still work as they currently do.

If any of the following happen after a change in the message definition, your code will no longer compile:

• A new field gets added to the message
• An old field gets removed (of course)
• An old field gets renamed
• An old field moves to a different location within the message definition

That last bullet point is probably not desirable, but I think it's a price worth paying for people who care very much about having compile-time guarantees that all their fields are fully initialized.

There's a non-trivial amount of boilerplate code required to make this work, but that shouldn't be any trouble for headers that are being automatically generated. If we have any concerns about unnecessary code bloat or increased compile times, we could put the classes for this opinionated builder pattern in a separate header from the usual public API header.

• Implemented in Add a utility for rigorously initializing a message instance #448.

[dirk-thomas]

Feedback from a review meeting on Mar 20th with the following attendees: Michael Carroll, Karsten Knese, Jacob Perron, Louise Poubel, Dirk Thomas, William Woodall.

Directories and Files - include

• Use cpp_detail subdirectory under msg / srv / action for the non-entry point headers. The subdirectory name isn't being reflected in the symbol names.
  • Consider tick-tock for moving header files into a subdirectories if used by non-trivial amount of user code.
  • See move non-entry point headers into detail subdir #461 moving these headers into a detail subdirectory.

Directories and Files - resource

The implementation of the methods could be moved into .cpp files which would require each interface package to export a library.

• Consider performance impact.

Directories and Files - src

• Bounded vector code can't be moved to .cpp since it is templated.

Naming of API

All type traits are defined in the namespace rosidl_generator_traits.
Moving these into the rosidl_generator_cpp namespace seems more consistent and clarifies where they are being defined when being used.

The rational why rosidl_typesupport_cpp is different and should stay like that is that other packages contribute to the same namespace.

API signatures

Currently using a custom allocator for a message, passing that message using intra-process communication, and using a different allocator on the other side is broken if these allocators don't have the same memory layout.

• This needs to be addressed one way or the other:
  • Switch the default allocator to a polymorphic allocator (introduced in C++17 but it seems reasonable to implement with C++14).
  • None of the rclcpp API works (compiles) for messages using custom allocators. As such the above use case we thought is problematic isn't possible atm. This needs to be revisited in the future - either by supporting messages using custom allocators in the rclcpp API or removing that option. See none of the API compiles for message types using a custom allocator rclcpp#1061.
  • Consider removing custom allocator support for messages all together.
    • Since allocators are usable when using the stucts as data objects we won't remove the allocator atm.

[dirk-thomas]

All bullet addressed or ticketed.