Add line break after first open paren in multiline function call

[rotu]

Update line break behavior to match the developer guide:

https://index.ros.org/doc/ros2/Contributing/Developer-Guide/

When a function call cannot fit on one line, wrap at the open parenthesis (not in between arguments) and start them on the next line with a 2-space indent. Continue with the 2-space indent on subsequent lines for more arguments. (Note that the Google style guide is internally contradictory on this point.)

[dirk-thomas]

The change itself looks good. But testing the existing code base with this shows numerous places where we don't adhere to the rule: https://ci.ros2.org/job/ci_linux/7579/testReport/

Before this can be merged those would need to be addressed so that the tests pass cleanly.

[rotu]

Ouch! So I guess the questions to ask:

1. Are there any other changes to the ament_uncrustify rules that should take place at this time so we only need to do this once?
2. Are there any shortcuts, or do I have to just open a bajillion PRs?

[dirk-thomas]

Are there any other changes to the ament_uncrustify rules that should take place at this time so we only need to do this once?

I don't know for sure - I haven't checked. Feel free to either look into that or just land these changes for now (I would think if there are others they shouldn't be as invasive).

Are there any shortcuts, or do I have to just open a bajillion PRs?

The number of PRs should not exceed the number of repos - but that number will likely be double digit 😒 I don't think there is a way to reduce that.

[rotu]

Blech. I've really made a mess of this. It looks like we have to keep indent_paren_open_brace false.

[hidmic]

Alright, I gave this a shot. Some conclusions and a request for feedback follow.

Reconciling clang-format with the code style that uncrustify currently enforces throughout our code base has not been possible. It's simply not as flexible and configurable as uncrustify is (e.g. uncrustify allows one to ignore details altogether whereas clang-format usually does not). Thus, it became a matter of getting ament_clang_format's default configuration as close as possible to what we have right now while not violating the GSG nor the ROS2 C++ style guide, and hopefully adapt uncrustify's right after.

I think I've managed to do the former. Here you can find the modified .clang-format configuration file and here is rclcpp re-formatted. I'm still far from a fully compatible uncrustify configuration but it'd be good for someone else to take a rough look at the results before investing more time.

[dirk-thomas]

Here you can find the modified .clang-format configuration file

The diff is unnecessary difficult to read. Before the file only contained the config values which were different from the based-on-style. Your diff re-adds a lot of configuration keys even though they only use the same value as defined in the based-on-style. Please share an actual diff of options which have changed to make it easier to review what options had to be modified. It would also be good to have an example for each changed option where / how it applies to the code base to see why how it affects it (comparing the old and the new format).

[hidmic]

True, sorry about that. Too many combinations.

I've removed all redundant settings from the .clang-format patch.

Applying the existing .clang-format configuration file yields this diff. Each option has the following effects on said patch:

• SpaceAfterTemplateKeyword: false (diff)
• BinPackArguments: false (diff)
• BinPackParameters: false (diff)
• BreakBeforeTernaryOperators: false (diff)
• AllowShortFunctionsOnASingleLine: None (diff)

Given that settings that Keep... do not enforce anything but keep what's already there, these cannot be applied incrementally. Reformatting master yields:

• KeepEmptyLinesAtTheStartOfBlocks: true + MaxEmptyLinesToKeep: 1 (diff)

[rotu]

@hidmic I think maybe this should be moved to its own PR

[ivanpauno]

Closing as this have been opened for a while and there haven't been any further movement.
A PR adjusting linters closer to the developer guide that doesn't create false positives, together with PRs fixing the places where the code wasn't following the style guide correctly, will be welcomed.