Skip to content

introduce RMW_EVENT_TYPE_MAX in rmw_event_type_t.#518

Merged
fujitatomoya merged 3 commits intorollingfrom
fujitatomoya/return-zero-structure
Feb 5, 2025
Merged

introduce RMW_EVENT_TYPE_MAX in rmw_event_type_t.#518
fujitatomoya merged 3 commits intorollingfrom
fujitatomoya/return-zero-structure

Conversation

@fujitatomoya
Copy link
Copy Markdown
Contributor

@fujitatomoya fujitatomoya commented Oct 16, 2024

depends on ros2/rmw#380

fujitatomoya
fujitatomoya commented Oct 16, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reference why we change this: ros2/rmw#380 (comment)

@fujitatomoya fujitatomoya mentioned this pull request Oct 16, 2024
Merged
@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 16, 2024

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/a5a27e64c97e3b687ed9c85018393aef/raw/398849da025367226392ce4eacd4f8114a09b768/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14711

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

This was referenced Oct 16, 2024
Merged
Merged
@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 17, 2024

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/6abc026e02c2c115a589c04c79e23c39/raw/398849da025367226392ce4eacd4f8114a09b768/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14714

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

eboasson
eboasson reviewed Oct 17, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fujitatomoya ! It is fine as it is but there is one thing I would do slightly differently: I would either also add it to rmw_take_event to maintain the pattern, or I would remove RMW_EVENT_INVALID there because init_rmw_event can never produce an rmw_event_t with that event type.

I don't like defaults in switches over enums, precisely because it prevents the compiler from helping me checking all relevant locations. I'm pretty sure if the default: unreachable wasn't present in rmw_take_event, you'd have added a case there, too 🙂

So I lean towards the former of my two suggestions, add RMW_EVENT_TYPE_MAX, and then also drop the unnecessary default case.

That said, there are more of those defaults ... Process-wise, dropping those unnecessary defaults might be better done in a separate PR. What do you think?

@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 17, 2024

@eboasson

I don't like defaults in switches over enums, precisely because it prevents the compiler from helping me checking all relevant locations. I'm pretty sure if the default: unreachable wasn't present in rmw_take_event, you'd have added a case there, too

so true. i would have added a case there...

So I lean towards the former of my two suggestions, add RMW_EVENT_TYPE_MAX, and then also drop the unnecessary default case.

agree.

That said, there are more of those defaults ... Process-wise, dropping those unnecessary defaults might be better done in a separate PR. What do you think?

agree too.

thanks!

@fujitatomoya fujitatomoya mentioned this pull request Oct 17, 2024
Closed
@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 17, 2024

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/82891bb189950fefcf68c0f191365af1/raw/398849da025367226392ce4eacd4f8114a09b768/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14720

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 18, 2024

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/16961ebf1f4059173817c9730b42d685/raw/398849da025367226392ce4eacd4f8114a09b768/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14722

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 19, 2024 •
edited
Loading

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/065211cb8895027ee0a710a50b6a9851/raw/398849da025367226392ce4eacd4f8114a09b768/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14726

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

eboasson
eboasson approved these changes Oct 22, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Oct 22, 2024

@eboasson thanks for the review, CI is all green so just waiting for ros2/rmw#380 lgtm.

@fujitatomoya
introduce RMW_EVENT_TYPE_MAX in rmw_event_type_t.
d5ae964 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
add a case for RMW_EVENT_TYPE_MAX to rmw_take_event().
46a3ad9 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya
remove redundant blank lines, make cpplint happy.
b6b8247 
Signed-off-by: Tomoya Fujita <Tomoya.Fujita@sony.com>
@fujitatomoya fujitatomoya force-pushed the fujitatomoya/return-zero-structure branch from 59b391f to b6b8247 Compare February 4, 2025 22:21
@fujitatomoya
Copy link
Copy Markdown
Contributor Author

fujitatomoya commented Feb 4, 2025 •
edited
Loading

Pulls: ros2/rmw#380, ros2/rmw_connextdds#162, ros2/rmw_fastrtps#785, #518
Gist: https://gist.githubusercontent.com/fujitatomoya/29f611cd228c356fd94dc372b220b0ba/raw/5dc544053744c46415545fe90336becf878d9765/ros2.repos
BUILD args: --packages-above-and-dependencies rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
TEST args: --packages-above rmw rmw_fastrtps_shared_cpp rmw_connextdds_common rmw_cyclonedds_cpp
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/15120

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@fujitatomoya fujitatomoya merged commit 45eebe6 into rolling Feb 5, 2025
@fujitatomoya fujitatomoya mentioned this pull request Dec 19, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eboasson eboasson eboasson approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

Assignees

@clalancette clalancette

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fujitatomoya @eboasson @clalancette