This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jrmolin on May 7 2022, 10:00 AM.

Details

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

Diff Detail

Event Timeline

jrmolin created this revision.May 7 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 10:00 AM
jrmolin requested review of this revision.May 7 2022, 10:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2022, 10:00 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jrmolin updated this revision to Diff 501836.Mar 2 2023, 5:25 AM

Updated the version string in the docs and pulled in main.

I finally found the correct CodeOwners.rst file.

jrmolin updated this revision to Diff 502112.Mar 3 2023, 6:04 AM

Finally figured out how to run the latest git-clang-format on the code

Please add unit tests to relevant files in clang/unittests/Format/.

Please reupload with the complete context.

Please add tests.

clang/include/clang/Format/Format.h
827

That's not a valid declaration (missing return type), so not a good example.

831

Please sort alphabetically. (So above AlwaysBreakBeforeMultilineStrings.)

831

You are missing the \since 17.

clang/lib/Format/Format.cpp
1510

Only set it in getLLVMStyle, which you are missing right now. Every other style will inherit it.

clang/lib/Format/TokenAnnotator.cpp
4740–4742

Merge the ifs.

And please add an entry to the ReleaseNotes.rst.

I am doing this for my team, which writes the security endpoint for Elastic Defend. The code is currently private, though the binaries are free to download and run. The number of contributors is around 30, and the lines of code is in the 100Ks (around 500K). I have not found a way to accomplish what this does with the available options. I am adding tests and am happy to maintain this. It is a rather small addition, so it is quite simple to keep updated.

jrmolin marked 4 inline comments as done.Mar 14 2023, 11:47 AM
jrmolin added inline comments.
clang/include/clang/Format/Format.h
827

fixed. creating a new patch with all the requested context.

831

\version 17? I added this locally and am generating a new patch, with all the requested context.

831

sorted. creating a patch with requested context

clang/lib/Format/TokenAnnotator.cpp
4740–4742

I'm not sure I collapsed what you wanted. I am generating a new patch with all the context.

jrmolin updated this revision to Diff 505215.Mar 14 2023, 12:24 PM
jrmolin marked 3 inline comments as done.

respond to code review requests/comments

MyDeveloperDay requested changes to this revision.Mar 14 2023, 4:15 PM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
1372

Regererate

1376

Ok you wrote this by hand it’s auto generated from format.h

clang/lib/Format/Format.cpp
872–874

Needs a parse test

clang/unittests/Format/FormatTest.cpp
25361

Please write it out long form

This revision now requires changes to proceed.Mar 14 2023, 4:15 PM

I am doing this for my team, which writes the security endpoint for Elastic Defend. The code is currently private, though the binaries are free to download and run. The number of contributors is around 30, and the lines of code is in the 100Ks (around 500K). I have not found a way to accomplish what this does with the available options. I am adding tests and am happy to maintain this. It is a rather small addition, so it is quite simple to keep updated.

Do you "have a publicly accessible style guide"?

I am doing this for my team, which writes the security endpoint for Elastic Defend. The code is currently private, though the binaries are free to download and run. The number of contributors is around 30, and the lines of code is in the 100Ks (around 500K). I have not found a way to accomplish what this does with the available options. I am adding tests and am happy to maintain this. It is a rather small addition, so it is quite simple to keep updated.

Do you "have a publicly accessible style guide"?

Unfortunately, we don't. I can create one in the likeness of the others, but I'm not sure where it will get published. I will talk to my managers about publishing a style doc.

clang/docs/ClangFormatStyleOptions.rst
1372

How do I re-generate? I'm happy to! I just don't see where to do that.

clang/lib/Format/Format.cpp
872–874

I will add one. Thanks for the reminder!

clang/unittests/Format/FormatTest.cpp
25361

What do you mean by "long form"?

MyDeveloperDay added inline comments.Mar 16 2023, 2:20 PM
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 updated this revision to Diff 507539.Mar 22 2023, 4:14 PM

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 marked 4 inline comments as done.Mar 22 2023, 4:16 PM

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

clang/docs/ClangFormatStyleOptions.rst
3663

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 added inline comments.Mar 23 2023, 2:22 AM
clang/docs/ClangFormatStyleOptions.rst
3663

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

3699

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

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

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.

clang/lib/Format/TokenAnnotator.cpp
4742–4748

That I meant with merge.

jrmolin updated this revision to Diff 508639.Mar 27 2023, 7:08 AM
jrmolin marked 2 inline comments as done.

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

jrmolin marked an inline comment as done.Mar 27 2023, 7:09 AM
jrmolin added inline comments.
clang/lib/Format/TokenAnnotator.cpp
4742–4748

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

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
4742–4748

I think now you need to format it.

jrmolin updated this revision to Diff 509317.Mar 29 2023, 5:07 AM

ran the formatter, ran the documentation generator.

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
4742–4748

I ran the formatting

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...

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
25367–25372
This comment was removed by 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
4743

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

MyDeveloperDay added inline comments.Mar 30 2023, 3:33 AM
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

MyDeveloperDay requested changes to this revision.Mar 30 2023, 3:33 AM
This revision now requires changes to proceed.Mar 30 2023, 3:33 AM
jrmolin updated this revision to Diff 512447.Apr 11 2023, 7:24 AM
jrmolin marked 3 inline comments as done.
  • 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 marked 2 inline comments as done.Apr 11 2023, 7:25 AM
jrmolin added inline comments.
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
4743

updated the tests.

clang/unittests/Format/FormatTest.cpp
25367–25372

Oh, I see. Sure!

jrmolin updated this revision to Diff 512449.Apr 11 2023, 7:29 AM
jrmolin marked an inline comment as done.
  • updated the documentation to show the option works for function declaration and definition
jrmolin updated this revision to Diff 512480.Apr 11 2023, 8:53 AM

fix a comment in the documentation

jrmolin marked an inline comment as done.Apr 11 2023, 8:56 AM
MyDeveloperDay retitled this revision from Add a new clang-format option AlwaysBreakBeforeFunctionParameters to [clang-format] Add a new clang-format option AlwaysBreakBeforeFunctionParameters.Apr 12 2023, 5:48 AM

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 edited the summary of this revision. (Show Details)Apr 12 2023, 6:29 AM

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 added inline comments.Apr 13 2023, 7:02 AM
clang/lib/Format/Format.cpp
873

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

1330

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
25358

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

25390

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

jrmolin updated this revision to Diff 513610.Apr 14 2023, 8:34 AM

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

jrmolin marked 4 inline comments as done.Apr 14 2023, 8:40 AM
jrmolin added inline comments.
clang/lib/Format/Format.cpp
1330

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
25358

I consolidated the tests and expanded.

jrmolin updated this revision to Diff 513717.Apr 14 2023, 12:37 PM
jrmolin marked 2 inline comments as done.

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 marked an inline comment as done and an inline comment as not done.Apr 21 2023, 8:30 AM
jrmolin updated this revision to Diff 515756.Apr 21 2023, 8:41 AM

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?

clang/include/clang/Format/Format.h
816

Can you format this as we do the other documentation?

jrmolin updated this revision to Diff 516450.Apr 24 2023, 10:09 AM
jrmolin marked an inline comment as done.

change formatting of enum options

jrmolin marked an inline comment as done.Apr 24 2023, 10:09 AM
jrmolin updated this revision to Diff 516452.Apr 24 2023, 10:12 AM

pulled upstream/main

MyDeveloperDay added inline comments.May 1 2023, 7:29 AM
clang/lib/Format/Format.cpp
347

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

jrmolin updated this revision to Diff 519293.May 3 2023, 4:09 PM

more code review updates. pulled main.

jrmolin marked an inline comment as done.May 3 2023, 4:11 PM
H-G-Hristov added inline comments.
clang/unittests/Format/FormatTest.cpp
25411

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

jrmolin added inline comments.Jun 13 2023, 7:28 AM
clang/unittests/Format/FormatTest.cpp
25411

How do you want me to test it? I'm happy to, if that means this will get merged at some point.

@jrmolin is this option really for breaking before the first parameter? Or are you trying to have one parameter per line as shown by the examples in your GitHub issue? Also, how would it interact with BAS_AlwaysBreak, BinPackParameters, and AllowAllParametersOfDeclarationOnNextLine? What if there is only one parameter? And do we really want something like the following for example?

template <typename T>
void baz(
    T t) requires C<T>
clang/include/clang/Format/Format.h
812–814
816

Can you add an example to show that the default penalty would break before the first parameter?

837
clang/lib/Format/ContinuationIndenter.cpp
357

Unrelated?

588
590

What would happen if there‘s a comment between the l_paren and the parameter?

1063–1066

Please reflow the comments.

clang/lib/Format/TokenAnnotator.cpp
4740–4742

Reflow comments.

4743

Again, we need to handle the case where a comment exists between the l_paren and the parameter.

4745–4753

Can you refactor the switch statement as it's also used in ContinuationIndenter.cpp above?