Page MenuHomePhabricator

[clang-format] Refactor SpaceBeforeParens to add options
ClosedPublic

Authored by crayroud on Sep 30 2021, 7:29 AM.

Details

Summary

The coding style of some projects requires to have more control on space before opening parentheses.
The goal is to add the support of clang-format to more projects.
For example adding a space only for function definitions or declarations.
This revision adds SpaceBeforeParensOptions to configure each option independently from one another.

# Example of usage:
SpaceBeforeParens: Custom
SpaceBeforeParensOptions:
  AfterControlStatements: true
  AfterFunctionDefinitionName: true

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
crayroud requested review of this revision.Sep 30 2021, 7:29 AM

First impressions is ControlStatementsAndFunctionDefinitionsExceptControlMacros is a mouthful...

I feel this is going the way of BraceWrapping in that it should be:

SpaceBeforeParens:
   - BeforeMacro: false
   - BeforeFunction: true

I feel this is going the way of BraceWrapping in that it should be:

SpaceBeforeParens:
   - BeforeMacro: false
   - BeforeFunction: true

So replace the enum with a struct? Yeah I think that's the way. For what it is here proposed it seems that there needs to be a BeforeFunctionDeclaration and BeforeFunctionDefinition.

It should also make the code easier.

MyDeveloperDay added inline comments.Oct 1 2021, 2:59 AM
clang/docs/ClangFormatStyleOptions.rst
3654

Now I look back here, why where these Macro considered the same as for loops? why would we want

for (....)
Q_FOREACH(...)

So this really does need a struct or we'll be eventually be adding

SBPO_ControlStatementsAndFunctionDefinitionsExceptControlMacrosButNotIfAndDefinatelyWhilesAndSometimesSwitchButOnlyOnTheSecondThursdayOfTheMonth

SpaceBeforeParen:
     AfterFunctionDefinitionName: false
     AfterFunctionDeclarationName: true
     AfterSwitch: true
     AfterForeachMacros: false
     .... (there are likely others)

`

crayroud added inline comments.Oct 1 2021, 3:12 AM
clang/docs/ClangFormatStyleOptions.rst
3654

Indeed replacing the enum with a struct as suggested is better to support the different possible combinations, compare to the current version of SpaceBeforeParens that results in very long names.

To support existing configuration files, I propose to keep the enum and to add a struct to handle the custom use cases and to cleanup the code. What do you think ?

SpaceBeforeParens: Custom
SpaceBeforeParensCustom:
     AfterFunctionDefinitionName: false
     AfterFunctionDeclarationName: true
     AfterSwitch: true
     AfterForeachMacros: false
    …

The use of "Custom" would kind of match the BraceWrapping, as for SpaceBeforeParensCustom either that or SpaceBeforeParensStyles , SpaceBeforeParensSettings, SpaceBeforeParensOptions I guess I don't really mind, (I always find naming things hard!) maybe the others might chip in on their preference of names

BreakBeforeBraces: Custom
BraceWrapping:
    AfterClass: true
    AfterControlStatement: false
    AfterEnum: true
    BeforeElse: true
    BeforeCatch: true
    AfterFunction: true
    AfterNamespace: true
    IndentBraces: false
    AfterStruct: true
    AfterUnion: true

Just adding a few more clang-format big guns as reviewers, its probably good to get some more input before embarking on what is going to probably be a reasonably sized change.

clang/docs/ClangFormatStyleOptions.rst
3654

I haven't looked too deep into the parsing, but if we could try to parse it as a struct and if that fails as enum for compatibility I would be in favor of that. But a custom is also acceptable.

owenpan added inline comments.Oct 3 2021, 4:46 PM
clang/docs/ClangFormatStyleOptions.rst
3654

If possible, we should deprecate the enum values and match them to the new struct values for backward compatibility without resorting to a Custom sub-option. See PackConstructorInitializers for an example.

Thank you for your inputs. I will work on the changes and update the patch once it is ready.

crayroud retitled this revision from [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens to [clang-format] Refactor SpaceBeforeParens to add flags.Oct 21 2021, 12:55 AM
crayroud edited the summary of this revision. (Show Details)
crayroud updated this revision to Diff 381168.Oct 21 2021, 1:11 AM

This new version adds the possibility to configure space before opening parentheses independently from one another. Instead of creating a new SBPO_ mode for each combinations.

Fundamentally this looks ok to me, the biggest concern is fathoming out the change in TokenAnnotator.cpp to mean the same thing, but I think that is what the tests should be for I think.

One think I do is use the output of this

https://clang.llvm.org/docs/ClangFormattedStatus.html

it creates a file in clang/docs/tools/clang-formatted-files.txt these are all the files that when we last checked were 100% clang-formatted (all 7949) of them..

Now be careful because people don't always maintain that clean status (naughty them!), but I use to ensure I'm not breaking clang-format (for at least the default LLVM style)

so build your own clang-format and then in the llvm-project directory I run

clang-format -verbose -n -files clang/docs/tools/clang-formatted-files.txt

This will check all the files (reasonably quickly YMMV)

$ clang-format -verbose -n -files clang/docs/tools/clang-formatted-files.txt
Clang-formating 7950 files
Formatting [1/7949] clang/bindings/python/tests/cindex/INPUTS/header1.h
Formatting [2/7949] clang/bindings/python/tests/cindex/INPUTS/header2.h
Formatting [3/7949] clang/bindings/python/tests/cindex/INPUTS/header3.h
Formatting [4/7949] clang/examples/Attribute/Attribute.cpp
Formatting [5/7949] clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp
Formatting [6/7949] clang/include/clang/Analysis/AnalysisDiagnostic.h
Formatting [7/7949] clang/include/clang/Analysis/BodyFarm.h
....

if your (or they more likely) have broken anything then you'll get a warning (in this case it was their fault)

Formatting [134/7949] clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:19:16: warning: code should be clang-formatted [-Wclang-format-violations]
namespace clang{

But this is a good way of giving clang-format a quick check to ensure its not broken anything (over large code base) - You will get failures (as this file is out of date)

clang/docs/ClangFormatStyleOptions.rst
3708

I'm not a massive fan of the use of 'Flags' in the config (I know we use it as the typename), naming things is hard!

clang/include/clang/Format/Format.h
3501

I'm not a massive fan of the word Flags here and thoughts?

clang/unittests/Format/FormatTest.cpp
14193

IMHO I think we should see tests for the other combinations of custom (I know it might be repeated)

clang/include/clang/Format/Format.h
3415

Please remove again. :)

3426

Could you sort these? AfterControlStatements e.g. would be in front of this.

3446

Should new be handled differently than the other operators? But at least add an operator example which is not new, and keep this one.

3490

Custom

3501

Yeah, neither, but Options is taken.

But Flags indicate that these will always be booleans, and we know that may change in the future.

Is it possible the reuse SpaceBeforeParensOptions as struct and still parse the old enum? (Yes of course, but is it feasible in terms of needed work?)

clang/lib/Format/Format.cpp
1217

Is this needed? Shouldn't the expand take care of that?

crayroud added inline comments.Oct 22 2021, 2:37 AM
clang/include/clang/Format/Format.h
3415

I changed the documentation to have a space added or removed only before the for loop colon. Indeed the space after the for is handled by SpaceBeforeParens... and not SpaceBeforeRangeBasedForLoopColon. Why do you think it is better to keep it as before ?

clang/lib/Format/Format.cpp
1217

Indeed the expand takes care of that and it would be nice not to have to repeat it here.
I had to add it for the test FormatTest.ConfigurationRoundTripTest, as it is done for LLVMStyle.BraceWrapping.
What do you think is best ?

Fundamentally this looks ok to me, the biggest concern is fathoming out the change in TokenAnnotator.cpp to mean the same thing, but I think that is what the tests should be for I think.

One think I do is use the output of this

https://clang.llvm.org/docs/ClangFormattedStatus.html

it creates a file in clang/docs/tools/clang-formatted-files.txt these are all the files that when we last checked were 100% clang-formatted (all 7949) of them..

Now be careful because people don't always maintain that clean status (naughty them!), but I use to ensure I'm not breaking clang-format (for at least the default LLVM style)

so build your own clang-format and then in the llvm-project directory I run

clang-format -verbose -n -files clang/docs/tools/clang-formatted-files.txt

This will check all the files (reasonably quickly YMMV)

$ clang-format -verbose -n -files clang/docs/tools/clang-formatted-files.txt
Clang-formating 7950 files
Formatting [1/7949] clang/bindings/python/tests/cindex/INPUTS/header1.h
Formatting [2/7949] clang/bindings/python/tests/cindex/INPUTS/header2.h
Formatting [3/7949] clang/bindings/python/tests/cindex/INPUTS/header3.h
Formatting [4/7949] clang/examples/Attribute/Attribute.cpp
Formatting [5/7949] clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp
Formatting [6/7949] clang/include/clang/Analysis/AnalysisDiagnostic.h
Formatting [7/7949] clang/include/clang/Analysis/BodyFarm.h
....

if your (or they more likely) have broken anything then you'll get a warning (in this case it was their fault)

Formatting [134/7949] clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h
clang/include/clang/Tooling/DependencyScanning/DependencyScanningTool.h:19:16: warning: code should be clang-formatted [-Wclang-format-violations]
namespace clang{

But this is a good way of giving clang-format a quick check to ensure its not broken anything (over large code base) - You will get failures (as this file is out of date)

clang/include/clang/Format/Format.h
3501

but "Options" as in SpaceBeforeParensOptions is the type not the name so we could use SpaceBeforeParensOptions

personally I would change the typename of SpaceBeforeParensOptions to avoid confusion but its not essential as it would be

SpaceBeforeParensCustom SpaceBeforeParensOptions;

for me I sometimes like using Struct anyway

SpaceBeforeParensStruct SpaceBeforeParensOptions;

clang/include/clang/Format/Format.h
3415

I must admit, I did not notice that the space is for the colon not the paren.

But nevertheless this is an unrelated change. You are very welcome to fix it, in its own commit.

clang/lib/Format/Format.cpp
1217

If that's the case, than it may be so.

crayroud added inline comments.Oct 25 2021, 3:09 AM
clang/lib/Format/TokenAnnotator.cpp
3171

I will simplify the following using Left.is(TT_FunctionDeclarationName)

crayroud updated this revision to Diff 383988.Nov 2 2021, 12:33 AM
crayroud retitled this revision from [clang-format] Refactor SpaceBeforeParens to add flags to [clang-format] Refactor SpaceBeforeParens to add options.
crayroud edited the summary of this revision. (Show Details)
  • SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions
  • All common space before parentheses are now grouped
  • More tests has been added
crayroud marked 6 inline comments as done.Nov 2 2021, 12:38 AM
crayroud added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3708

SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions.

clang/include/clang/Format/Format.h
3501

SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions. And the old SpaceBeforeParensOptions enum has been renamed to SpaceBeforeParensStyle.

clang/unittests/Format/FormatTest.cpp
14193

More tests has been added

crayroud marked 10 inline comments as done.Nov 2 2021, 12:43 AM

I used the following command to verify my changes on multiple files. Thank you for the tip.

clang-format -verbose -n -files clang/docs/tools/clang-formatted-files.txt

This looks pretty good to me. Could you add a release note into docs/ReleaseNote.rst (we have a .clang-format section)

MyDeveloperDay added inline comments.Nov 2 2021, 1:08 AM
clang/lib/Format/TokenAnnotator.cpp
3469

I'm ever so slightly struggling to see where this case is covered. Could you give me the line number?

crayroud added inline comments.Nov 2 2021, 1:15 AM
clang/lib/Format/TokenAnnotator.cpp
3469

Left.is(tok::comma) is used to always add a space after a coma, but we want to be able to configure the space after the coma in the following example:

bool operator,();

Verified by:

25201:   verifyFormat("bool operator,();");
crayroud updated this revision to Diff 383995.Nov 2 2021, 1:31 AM

Add a release note into clang/docs/ReleaseNotes.rst

crayroud marked an inline comment as done.Nov 2 2021, 2:51 AM

Looks good, and I really need this to land, to expand it. ;)

clang/include/clang/Format/Format.h
3369

Sort before AfterFunction

clang/unittests/Format/FormatTest.cpp
14275

Is this really desired?

crayroud updated this revision to Diff 384352.Wed, Nov 3, 12:19 AM
crayroud marked an inline comment as done.

Reorder the options

crayroud marked an inline comment as done.Wed, Nov 3, 12:19 AM
crayroud added inline comments.
clang/unittests/Format/FormatTest.cpp
14275

It is allowed to have a space after __attribute__ and the behaviour is the same with SpaceBeforeParens: NonEmptyParentheses.

To me this looks good, if the sorting is finalized. :)

Do you need someone to push this for you? If yes please state name and email, if not please wait a few days for other opinions.

clang/include/clang/Format/Format.h
3348

Last: This comes just before the Before, I is after F.

clang/unittests/Format/FormatTest.cpp
14275

Okay.

This revision is now accepted and ready to land.Wed, Nov 3, 1:34 PM
crayroud updated this revision to Diff 384679.Thu, Nov 4, 1:02 AM

Fix ordering of SpaceBeforeParensCustom options

crayroud marked an inline comment as done.Thu, Nov 4, 1:03 AM
This comment was removed by crayroud.
This comment was removed by crayroud.

Could you please wait before to push this changes to the main branch, there is one more point I want to verify.

I do not have commit access, could you please help with the push?

Here are the commit details: C. Rayroud - rayroudc@gmail.com

I do not have commit access, could you please help with the push?

Here are the commit details: C. Rayroud - rayroudc@gmail.com

Will push it on tuesday, if no one objects. I already have it in my local branch.

I'm looking at a bug (sorry can't log it as we don't seem to have a working bug tracker!) that causes a regression on operator new( and operator delete( which I believe I've tracked to this change

v12

~foo() { ::operator delete(bar); }

::operator delete(bar)

after this change, (space between delete and (bar) when at function depth, but not at global)

~foo() { ::operator delete (bar); }

::operator delete(bar)
MyDeveloperDay added inline comments.Mon, Nov 29, 1:00 AM
clang/lib/Format/TokenAnnotator.cpp
3161–3169

This is a change in behaviour for operators, it breaks LLVM style and we are going to either have to fix this or revert the whole change until we can decide what is best.

clang/unittests/Format/FormatTest.cpp
14165

are you testing the -ve case anywhere?

crayroud added inline comments.Mon, Nov 29, 8:24 AM
clang/unittests/Format/FormatTest.cpp
14165

What do you mean by "-ve case" ?