Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
jrmolin added inline comments.Mar 15 2023, 10:31 AM
clang/lib/Format/Format.cpp
885

I will add one. Thanks for the reminder!

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
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 added inline comments.Mar 23 2023, 2:22 AM
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

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
4879–4885

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

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 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
4879–4885

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
25672–25677
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
4880

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
4880

updated the tests.

clang/unittests/Format/FormatTest.cpp
25672–25677

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
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 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
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 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.Wed, May 3, 4:09 PM

more code review updates. pulled main.

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

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