[clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters

[jrmolin]

Summary

Add the definition, documentation, and implementation for a new clang-format option.

This is for new issue https://github.com/llvm/llvm-project/issues/62092

[jrmolin]

clang/lib/Format/Format.cpp

885

I will add one. Thanks for the reminder!

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

1372

docs/tools/dump_format_style.py but ensure you move your example to Format.h first or its going to wipe out these changes

[jrmolin]

I updated the code comment in Format.h and ran the docs generator. I didn't modify the Macros section, but that got updated when I ran the docs generator.

We don't have a published style guide, unfortunately. I can work towards that, but I don't know how long that would take. My team doesn't like to agree on formatting changes. Hence this patch. We rolled this into the 3.9.0 release, and have been stuck with that ever since.

[jrmolin]

I still don't know what Please write it out long form means for the unit test.

clang/docs/ClangFormatStyleOptions.rst

3748

I didn't modify this section in the header at all. I assume someone modified the Format.h file and didn't update the docs.

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

3748

that needs to be looked at, as to what changed that meant it changed and done separately.

3784

This won't generate without a /version, we need to get this fixed outside of this commit

clang/include/clang/Format/Format.h

823

16.0

[MyDeveloperDay]

if you go ahead an rebase your should get rG7a5b95732ade: [clang-format] NFC Format.h and ClangFormatStyleOptions.rst are out of date that will remove the need for the Macro section in the rst

[MyDeveloperDay]

Please rebase

[jrmolin]

In D125171#4215910, @MyDeveloperDay wrote:

if you go ahead an rebase your should get rG7a5b95732ade: [clang-format] NFC Format.h and ClangFormatStyleOptions.rst are out of date that will remove the need for the Macro section in the rst

So pull in that git hash and re-diff? I couldn't get arcanist installed cleanly (php issues), so I've been using git diff -U999999... to generate the patches. I just keep pulling down main before creating a patch.

[HazardyKnusperkeks]

clang/lib/Format/TokenAnnotator.cpp

4879–4885

That I meant with merge.

[jrmolin]

pulled main, re-generated the docs, then re-generated the diff. Also added a parse test and collapsed some nested if conditions.

[jrmolin]

clang/lib/Format/TokenAnnotator.cpp

4879–4885

Ah, understood. I generally don't collapse ifs, so I didn't know how much you wanted. It seems like ... all of it.

[HazardyKnusperkeks]

Looks good to me, not giving my formal approval since the ongoing discussion about the style guide.

clang/include/clang/Format/Format.h

823

Since when do we add the .0?

clang/lib/Format/TokenAnnotator.cpp

4879–4885

I think now you need to format it.

[jrmolin]

ran the formatter, ran the documentation generator.

[jrmolin]

I think I have hit all the requests now. We're in the middle of building release candidates and testing, so management is taking longer and longer to get back to me about a style guide for my team.

We added it, because it forces a consistent look across all function declarations. I don't know what the next steps are now. I'm stuck; you're stuck. :shrug:

clang/lib/Format/TokenAnnotator.cpp

4879–4885

I ran the formatting

[jrmolin]

In D125171#4230397, @jrmolin wrote:

I think I have hit all the requests now. We're in the middle of building release candidates and testing, so management is taking longer and longer to get back to me about a style guide for my team.

We added it, because it forces a consistent look across all function declarations. I don't know what the next steps are now. I'm stuck; you're stuck. :shrug:

I started a style guide at https://github.com/elastic/endpoint/blob/add_style_guide/style_guide.md, but it hasn't been approved or merged to main. It's at least a start...

[MyDeveloperDay]

I don't like using additional variables in unit tests, if someone changes 25400 multiple tests could break, each test should be stand alone in my view

clang/unittests/Format/FormatTest.cpp

25672–25677

This comment was removed by MyDeveloperDay.

[MyDeveloperDay]

Can you show your example from a implementation case and not just a declaration i.e. what happens with

int function1(int param1) {}

clang/lib/Format/TokenAnnotator.cpp

4880

can you test the () case in your tests please with AlwaysBreakBeforeFunctionParameters set to true

[MyDeveloperDay]

clang/docs/ClangFormatStyleOptions.rst

1374

here you say declaration but don't we want the same in definition too? otherewise should it be AlwaysBreakBeforeFunctionDefinitionParameters if you are expecting something else?

1380

This isn't relevant is it? AlwaysBreakAfterReturnType

[jrmolin]

• updated the tests to use long-form
• added new tests to verify the no-parameter cases
• updated the documentation to minimize formatting variables
• ran the style guide generator

[jrmolin]

clang/docs/ClangFormatStyleOptions.rst

1380

No, it's not relevant, but I saw it in other examples, and I like the way it looks. I can change the example to be more minimal.

clang/lib/Format/TokenAnnotator.cpp

4880

updated the tests.

clang/unittests/Format/FormatTest.cpp

25672–25677

Oh, I see. Sure!

[jrmolin]

• updated the documentation to show the option works for function declaration and definition

[jrmolin]

fix a comment in the documentation

[MyDeveloperDay]

is it possible to point to a github issue that this related to in the review summary, if not please can you make one so we can track the issue its trying to solve

[jrmolin]

In D125171#4261301, @MyDeveloperDay wrote:

is it possible to point to a github issue that this related to in the review summary, if not please can you make one so we can track the issue its trying to solve

added an issue: https://github.com/llvm/llvm-project/issues/62092

[MyDeveloperDay]

clang/lib/Format/Format.cpp

886

This should be Alphabetic should this be before the MultlineStrings option?

1348

ok, we have this think that the lifetime of an option goes from bool -> enum -> struct, sometimes we pick up early that true/false aren't good enough

so here is the think... AlwaysBreakBeforeFunctionParameters should this be an enum and BreakBeforeFunctionParameters but with values

Leave, Never, Always

i.e. does AlwaysBreakBeforeFunctionParameters = false mean never break? or sometimes break.

We don't really want "false" to mean do something..we want it to mean don't do anything i.e. Leave

clang/unittests/Format/FormatTest.cpp

25663

I would say all these function could go to the single format of verifyFormat

25695

you can just use the single form of verifyFormat() it will call messUp to remove the whitespace

[jrmolin]

changing the added option from a boolean to an enum that takes Leave, Always, and Never.

[jrmolin]

clang/lib/Format/Format.cpp

1348

Please let me know if I did this wrong! This took more than I was expecting. I added parsing for "false" and "true" to be "Leave" and "Always". That is kind of confusing, and there is no need for "backwards compatibility" in this, but it looked easy. Should I remove that?

clang/unittests/Format/FormatTest.cpp

25663

I consolidated the tests and expanded.

[jrmolin]

there were a couple of bugs in the last patch that I missed. format tests all pass again with this one.

I am using "Leave" as a transparent value, so no line breaks are added and none are removed -- the penalty calculus drives. Just like it was before this patch. It's the best I can do for making users not have to care about this option.

[jrmolin]

a unit test was failing, so I fixed it.

clang/include/clang/Format/Format.h

851

so should this be 17.0 now? This has gone on quite long.

clang/lib/Format/Format.cpp

345

I should probably delete these, right?

[MyDeveloperDay]

clang/include/clang/Format/Format.h

816

Can you format this as we do the other documentation?

[jrmolin]

change formatting of enum options

[jrmolin]

pulled upstream/main

[MyDeveloperDay]

clang/lib/Format/Format.cpp

347

yes remove these... this is why you need to mark the comments done

[jrmolin]

more code review updates. pulled main.

[H-G-Hristov]

clang/unittests/Format/FormatTest.cpp

25716

Does this work with BAS_BlockIndent? I don't see any related tests.