search

ament / ament_lint

Apache License 2.0
38 stars 107 forks source link

ament_clang_format: use open braces for enum definitions #426

Closed james-rms closed 2 years ago

james-rms commented 2 years ago

The ROS 2 C++ style guide is ambiguous about this:

  • Use open braces for function, class, and struct definitions, but cuddle braces on if, else, while, for, etc… Exception: when an if (or while, etc.) condition is long enough to require line-wrapping, then use an open brace (i.e., don’t cuddle).

It seems more consistent to also use open braces for enum declarations, ie:

enum struct Format
{
  OPTION_ONE,
  OPTION_TWO,
};
mjeronimo commented 2 years ago

I'll bring this up next week with the ROS 2 team and get back to you. Perhaps we should clarify the style guide as well.

Or, perhaps, @clalancette has input.

clalancette commented 2 years ago

I'll bring this up next week with the ROS 2 team and get back to you. Perhaps we should clarify the style guide as well.

I took a look through our code, and we are already using open-braces for enums. It's enforced by our uncrustify rules. We've also already updated the style guide in https://github.com/ros2/ros2_documentation/pull/3203 .

As far as I know, none of the ROS 2 core packages use ament_clang_format, so this shouldn't change anything there (though we'd want to do a full CI run just to be sure). The bigger concern is downstream packages that use this; they very well may get new warnings from this.

At the end of the day, I'm OK with that as that is the point of Rolling. But because of that, I don't think that this would be backportable to Humble or other releases.

So this gets a thumbs up from me with green CI, assuming there are no other technical problems.

mjeronimo commented 2 years ago
james-rms commented 2 years ago

FYI the reason I made this PR in the first place is that without AfterEnum: true, clang-format on Foxy and Galactic uses open braces for enums but Humble and Rolling use cuddled. See this CI run for evidence: https://github.com/ros-tooling/rosbag2_storage_mcap/actions/runs/3519952298/jobs/5900411262

For that reason I think it makes sense to backport this to Humble - this makes clang-format's behavior across Foxy, Galactic, Humble and Rolling the same.

That said, I see the concerns about breaking people's existing formatting and don't feel strongly about this, so it's up to you.