enable nl_func_call_start_multi_line in uncrustify

[dirk-thomas]

Follow up of #148.

This patch enforces that arguments not fitting in one line must start on a new line (as it is described in the ROS 2 development guide). While that requires quite a bit of existing code to be updated to be compliant the work is easily automated by calling ament_uncrustify --reformat <path>.

Unfortunately nobody on the original ticket was willing to update existing code to comply with the proposed change. So this PR (and the referenced ones) are doing that.

[jacobperron]

LGTM, with full green CI (linter tests).

[wjwwood]

One thing this does which is not great is it turns things like this:

some_func({
  1,
  2,
});

Into:

some_func(
{
  1,
  2,
});  

Which I think isn't helpful or more correct.

[jacobperron]

I thought that looked a little less good too, but I do think the pro (enforce line-break after paren) outweighs it.

[wjwwood]

I thought that looked a little less good too, but I do think the pro (enforce line-break after paren) outweighs it.

I can't say I really agree. I think it's frustrating to do things which don't make sense just because the linter is isn't smart enough to do the desired thing. And in the face of that I'd rather trust the developer and reviewers to get it right.

I approved this pr and several of the connected ones, but honestly I'm not sure it's a step in the right direction. It's a lot of line changes which disrupt backporting and rebasing (for people who maintain patches on top of our code) for a sub-optimal enforcement of the rule...

[dirk-thomas]

• Linux
• Linux-aarch64
• macOS (many test failures - all present in the nightlies)
• Windows (unrelated test failures)
• Windows-container (job type considered to be broken at the moment)

Not convinced it is a good direction.

[wjwwood]

Why merge some of the pull requests? What if one of the reviews on the remaining repositories turned up something that suggested we shouldn't use this setting?

[jacobperron]

Whether or not we move forward with enabling this rule, it has at least unveiled many instances where we were inconsistent with our style guide. So I think it's still worth fixing those, even though it will cause some churn.

[jacobperron]

FWIW, I thought that this style was documented in the style guide, but I can't find it. We do document the use of hanging indents for Python code, but not for C++. Considering that the style was prevalent the ROS 2 code base before this change (and now that the violations have been fixed), maybe we should consider adding a line about it to the style guide.

Nevermind. I see the quote in the original ticket. Just missed it.