search

ros2 / rmw_cyclonedds

ROS 2 RMW layer for Eclipse Cyclone DDS
Apache License 2.0
117 stars 90 forks source link

introduce RMW_EVENT_TYPE_MAX in rmw_event_type_t. #518

Open fujitatomoya opened 1 month ago

fujitatomoya commented 1 month ago

depends on https://github.com/ros2/rmw/pull/380

fujitatomoya commented 1 month ago

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, ros2/rmw_cyclonedds#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

fujitatomoya commented 1 month ago

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, ros2/rmw_cyclonedds#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

fujitatomoya commented 1 month ago

@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 commented 1 month ago

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, ros2/rmw_cyclonedds#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

fujitatomoya commented 1 month ago

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, ros2/rmw_cyclonedds#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

fujitatomoya commented 1 month ago

Pulls: ros2/rmw#380, ros2/rmw_fastrtps#785, ros2/rmw_connextdds#162, ros2/rmw_cyclonedds#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

fujitatomoya commented 1 month ago

@eboasson thanks for the review, CI is all green so just waiting for https://github.com/ros2/rmw/pull/380 lgtm.