[clang-format] Refactor SpaceBeforeParens to add options

[crayroud]

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

[MyDeveloperDay]

First impressions is ControlStatementsAndFunctionDefinitionsExceptControlMacros is a mouthful...

[MyDeveloperDay]

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

SpaceBeforeParens:
   - BeforeMacro: false
   - BeforeFunction: true

[HazardyKnusperkeks]

In D110833#3033849, @MyDeveloperDay wrote:

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]

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]

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
    …

[MyDeveloperDay]

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

[MyDeveloperDay]

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.

[HazardyKnusperkeks]

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]

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.

[crayroud]

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

[crayroud]

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.

[MyDeveloperDay]

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)

[HazardyKnusperkeks]

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]

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 ?

[MyDeveloperDay]

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;

[HazardyKnusperkeks]

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]

clang/lib/Format/TokenAnnotator.cpp

3171

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

[crayroud]

• SpaceBeforeParensFlags has been renamed to SpaceBeforeParensOptions
• All common space before parentheses are now grouped
• More tests has been added

[crayroud]

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]

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

[MyDeveloperDay]

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

[MyDeveloperDay]

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]

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]

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

[HazardyKnusperkeks]

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]

Reorder the options

[crayroud]

clang/unittests/Format/FormatTest.cpp

14275

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

[HazardyKnusperkeks]

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.

[crayroud]

Fix ordering of SpaceBeforeParensCustom options

[crayroud]

This comment was removed by crayroud.

[crayroud]

This comment was removed by crayroud.

[crayroud]

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

[crayroud]

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

Here are the commit details: C. Rayroud - [e-mail]

[HazardyKnusperkeks]

In D110833#3111632, @crayroud wrote:

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

Here are the commit details: C. Rayroud - [e-mail]

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

[crayroud]

git revision: 6facafe7da59eb02e8c1519670b503efda2f4b58

[MyDeveloperDay]

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]

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]

clang/unittests/Format/FormatTest.cpp

14165

What do you mean by "-ve case" ?