-
Notifications
You must be signed in to change notification settings - Fork 148
rosidl_generator_cpp: constexpr message traits and to_tuple_ref for generated structs #928
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rolling
Are you sure you want to change the base?
Conversation
fujitatomoya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this is useful and non-breaking change, lgtm with green CI.
i also think that alternative approach would be traits class (like rosidl_generator_traits) rather than adding members to the message itself. this keeps the message "cleaner" but requires more boilerplate for users.
a couple of comments,
(not blocking this PR) Does this also apply to service request/response structs and action types? The issue only mentions messages.
Users won't know this feature exists without documentation. Should be mentioned in the rosidl_generator_cpp README or official documentation?
Yes, good point. What about something like the following in namespace rosidl_generator_traits
{
template<>
struct MessageTraits<rosidl_generator_tests::msg::Defaults> {
static constexpr std::size_t member_count = 13;
static constexpr std::array<std::string_view, member_count> member_names = {"bool_value", ...};
}
}I also propose the following addition, as this function greatly reduces the amount of boilerplate needed to do generic programming over the structs. Without it, one must implement a structured binding to tuple function for each possible number of members. It works well, but a maximum member count has to be decided on, which is avoided by defining the function below. namespace rosidl_generator_traits
{
template<typename T, std::enable_if_t<std::is_same_v<std::decay_t<T>, rosidl_generator_tests::msg::Defaults>, int> = 0>
constexpr auto as_tuple_ref(T&& msg) {
return std::forward_as_tuple(std::forward<T>(msg).bool_value, ..., std::forward<T>(msg).uint64_value);
}
}
When defined in the
Yes, should probably add some documentation once we agree on the design. |
|
As per Waffle meeting this needs to go to the PMC. |
|
Great, I've updated the example above to use forward_as_tuple, in order to be more flexible. I will convert this to a draft, and revise the implementation after the PMC has looked at it. |
|
When this was raised in last week's PMC the consensus seemed to be that the change looks really useful. Thanks for putting up this PR. Looks like all checks pass, and @fujitatomoya has already given feedback. So, I'm happy to mark it LGTM. Since |
|
@asymingt I marked it as draft, because this PR does not yet contain the changes proposed by @fujitatomoya. I will update the PR and request another review. |
Signed-off-by: Øystein Sture <[email protected]>
8d74060 to
a60cf47
Compare
|
Ok, I've updated the PR with the proposed changes, please take a look. I was hoping to make a PR to follow up this one to change the message struct constructors to be constexpr, but that is not possible until C++20 due to the if/else. |
Signed-off-by: Øystein Sture <[email protected]>
|
This pull request has been mentioned on Open Robotics Discourse. There might be relevant details there: https://discourse.openrobotics.org/t/ros-pmc-minutes-for-january-27-2026/52151/1 |
I believe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM s.t a green build. Let's please update the documentation (PR against a separate repo) per my earlier comment.
|
@fujitatomoya, I'm going to hold of merging until you approve the changes you requested. Feel free to merge, if the CI above all goes 🟢 🟢 🟢 🟢 |
Description
This PR adds a constexpr-capable MemberTraits structure such that the number of members and member names can be used in a compile-time context. The PR also adds a function that forwards the members of the struct as a tuple of references. This makes it very simple to treat the members generically, using for example std::apply.
Implements #923
Is this user-facing behavior change?
It should not, as the new struct / function is contained within the appropriate namespaces.
Did you use Generative AI?
No