[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Christian Rayroud via Phabricator via cfe-commits]

crayroud added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3649
 ``SBPO_ControlStatementsExceptForEachMacros`` remains an alias for
 backward compatibility.
 

HazardyKnusperkeks wrote:
> crayroud wrote:
> > MyDeveloperDay wrote:
> > > 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)
> > > ```
> > >  
> > > `
> > > 
> > > 
> > > 
> > > 
> > > 
> > 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
> > …
> > ```
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3649
 ``SBPO_ControlStatementsExceptForEachMacros`` remains an alias for
 backward compatibility.
 

crayroud wrote:
> MyDeveloperDay wrote:
> > 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)
> > ```
> >  
> > `
> > 
> > 
> > 
> > 
> > 
> 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
> …
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Christian Rayroud via Phabricator via cfe-commits]

crayroud added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3649
 ``SBPO_ControlStatementsExceptForEachMacros`` remains an alias for
 backward compatibility.
 

MyDeveloperDay wrote:
> 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)
> ```
>  
> `
> 
> 
> 
> 
> 
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
…
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/docs/ClangFormatStyleOptions.rst:3649
 ``SBPO_ControlStatementsExceptForEachMacros`` remains an alias for
 backward compatibility.
 

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)
```
 
`







Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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

  SpaceBeforeParens:
 - BeforeMacro: false
 - BeforeFunction: true


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

First impressions is 
`ControlStatementsAndFunctionDefinitionsExceptControlMacros` is a mouthful...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110833/new/

https://reviews.llvm.org/D110833

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D110833: [clang-format] Add ControlStatementsAndFunctionDefinitionsExceptControlMacros option to SpaceBeforeParens

[Christian Rayroud via Phabricator via cfe-commits]

crayroud created this revision.
crayroud added reviewers: aaron.ballman, rsmith.
crayroud added projects: clang, clang-format.
crayroud requested review of this revision.

For some projects the coding style defined requires to have a space before 
opening parentheses for function definitions.

This revision adds the 
ControlStatementsAndFunctionDefinitionsExceptControlMacros option to 
SpaceBeforeParens which act as SBPO_ControlStatementsExceptControlMacros but 
also put a space before opening parentheses for function definitions.

The goal of this commit is too add the support of clang-format to these 
projects.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110833

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -2117,6 +2117,13 @@
"}",
Style);
 
+  Style.SpaceBeforeParens = FormatStyle::
+  SBPO_ControlStatementsAndFunctionDefinitionsExceptControlMacros;
+  verifyFormat("void f () {\n"
+   "  Q_FOREACH(Item *item, itemlist) {}\n"
+   "}",
+   Style);
+
   // As function-like macros.
   verifyFormat("#define foreach(x, y)\n"
"#define Q_FOREACH(x, y)\n"
@@ -14133,6 +14140,62 @@
   verifyFormat("X A::operator++ (T);", SomeSpace);
   verifyFormat("int x = int (y);", SomeSpace);
   verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
+
+  FormatStyle SomeSpace2 = getLLVMStyle();
+  SomeSpace2.SpaceBeforeParens = FormatStyle::
+  SBPO_ControlStatementsAndFunctionDefinitionsExceptControlMacros;
+
+  verifyFormat("int f();", SomeSpace2);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace2);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace2);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace2);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace2);
+  verifyFormat("A::A () : a(1) {}", SomeSpace2);
+  verifyFormat("void f() __attribute__((asdf));", SomeSpace2);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace2);
+  verifyFormat("#define A(x) x", SomeSpace2);
+  verifyFormat("#define A (x) x", SomeSpace2);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace2);
+  verifyFormat("auto i = std::make_unique(5);", SomeSpace2);
+  verifyFormat("size_t x = sizeof(x);", SomeSpace2);
+  verifyFormat("auto f(int x) -> decltype(x);", SomeSpace2);
+  verifyFormat("auto f(int x) -> typeof(x);", SomeSpace2);
+  verifyFormat("auto f(int x) -> _Atomic(x);", SomeSpace2);
+  verifyFormat("auto f(int x) -> __underlying_type(x);", SomeSpace2);
+  verifyFormat("int f(T x) noexcept(x.create());", SomeSpace2);
+  verifyFormat("alignas(128) char a[128];", SomeSpace2);
+  verifyFormat("size_t x = alignof(MyType);", SomeSpace2);
+  verifyFormat("static_assert(sizeof(char) == 1, \"Impossible!\");",
+   SomeSpace2);
+  verifyFormat("int f() throw(Deprecated);", SomeSpace2);
+  verifyFormat("typedef void (*cb)(int);", SomeSpace2);
+  verifyFormat("T A::operator()();", SomeSpace2);
+  verifyFormat("X A::operator++(T);", SomeSpace2);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace2);
+  verifyFormat("int x = int(y);", SomeSpace2);
+  verifyFormat("M (std::size_t R, std::size_t C) : C(C), data(R) {}",
+   SomeSpace2);
 }
 
 TEST_F(FormatTest, SpaceAfterLogicalNot) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -3138,12 +3138,18 @@
 if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
 (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
   return true;
-if (Style.SpaceBeforeParens ==
-FormatStyle::SBPO_ControlStatementsExceptControlMacros &&
+if ((Style.SpaceBeforeParens ==
+ FormatStyle::SBPO_ControlStatementsExceptControlMacros ||
+ Style.SpaceBeforeParens ==
+ FormatStyle::
+ SBPO_ControlStatementsAndFunctionDefinitionsExceptControlMacros) &&
 Left.is(TT_ForEachMacro))
   return false;
-if (Style.SpaceBeforeParens ==
-