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).