[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357344: [clang-format]: Add NonEmptyParentheses spacing 
option (authored by reuk, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55170?vs=191560=192969#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55170

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Format/TokenAnnotator.cpp
  cfe/trunk/lib/Format/TokenAnnotator.h
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -1690,6 +1690,17 @@
 ///}
 /// \endcode
 SBPO_ControlStatements,
+/// Put a space before opening parentheses only if the parentheses are not
+/// empty i.e. '()'
+/// \code
+///   void() {
+/// if (true) {
+///   f();
+///   g (x, y, z);
+/// }
+///   }
+/// \endcode
+SBPO_NonEmptyParentheses,
 /// Always put a space before opening parentheses, except when it's
 /// prohibited by the syntax rules (in function-like macro definitions) or
 /// when determined by other style rules (after unary operators, opening
Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -285,6 +285,8 @@
 IO.enumCase(Value, "Never", FormatStyle::SBPO_Never);
 IO.enumCase(Value, "ControlStatements",
 FormatStyle::SBPO_ControlStatements);
+IO.enumCase(Value, "NonEmptyParentheses",
+FormatStyle::SBPO_NonEmptyParentheses);
 IO.enumCase(Value, "Always", FormatStyle::SBPO_Always);
 
 // For backward compatibility.
Index: cfe/trunk/lib/Format/TokenAnnotator.h
===
--- cfe/trunk/lib/Format/TokenAnnotator.h
+++ cfe/trunk/lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine , const FormatToken ,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken ) const;
+
   bool spaceRequiredBetween(const AnnotatedLine , const FormatToken ,
 const FormatToken );
 
Index: cfe/trunk/lib/Format/TokenAnnotator.cpp
===
--- cfe/trunk/lib/Format/TokenAnnotator.cpp
+++ cfe/trunk/lib/Format/TokenAnnotator.cpp
@@ -2453,6 +2453,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken ) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2599,9 +2605,9 @@
  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
tok::kw_new, tok::kw_delete) &&
   (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
+   (spaceRequiredBeforeParens(Right) &&
 (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
+ Left.is(tok::r_paren) || Left.isSimpleTypeSpecifier() ||
  (Left.is(tok::r_square) && Left.MatchingParen &&
   Left.MatchingParen->is(TT_LambdaLSquare))) &&
 Line.Type != LT_PreprocessorDirective);
@@ -2795,7 +2801,7 @@
   Left.isOneOf(TT_TrailingReturnArrow, TT_LambdaArrow))
 return true;
   if (Right.is(TT_OverloadedOperatorLParen))
-return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
+return spaceRequiredBeforeParens(Right);
   if (Left.is(tok::comma))
 return true;
   if (Right.is(tok::comma))
@@ -2879,7 +2885,7 @@
 return true;
   if (Left.is(TT_TemplateCloser) && Right.is(tok::l_paren) &&
   Right.isNot(TT_FunctionTypeLParen))
-return Style.SpaceBeforeParens == FormatStyle::SBPO_Always;
+return spaceRequiredBeforeParens(Right);
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&
   Left.MatchingParen && Left.MatchingParen->is(TT_OverloadedOperatorLParen))
 return false;
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -9627,6 +9627,60 @@
   verifyFormat("T A::operator() 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek accepted this revision.
klimek added a comment.

Oh, and LG :)


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1439864 , @MyDeveloperDay 
wrote:

> @lebedev.ri Are we talking about a general ideology of the long term cost to 
> allow any new things in? or to not allow things in this specific case?
>
> because in this specific case all the changes are based on what is really a 
> single clause that was already there before, namely
>
>   Style.SpaceBeforeParens == FormatStyle::SBPO_Always
>
>
> Now that clause is abstracted away to be (the abstraction in my view being a 
> nice part of this change, becuase it avoided duplication and made it easier 
> to reason about!)
>
>   Style.SpaceBeforeParens == FormatStyle::SBPO_Always  ||
>   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && 
> Right.ParameterCount > 0);
>
>
> i.e. always add a space, or add a space only when the number of parameters is 
> > 0 and I don't want a space on empty parameters. (simple enough!)
>
> So whilst I understand the arguments of code complexity, I feel it is 
> sometimes used to discourage, what is in at least my view seemingly 
> reasonable requests. (not my choice but reasonable all the same)
>
> This was why I asked what was meant by "cost", but you raise the other cost, 
> and that is one that is more legitimate... or is it?. have we seen it here as 
> a problem or do we just fear it?


This adds another condition to an already complex "if" that I think literally 
nobody fully understands, and I think it does so for very questionable value 
(why does it matter whether there is a space diff between the arg and no-arg 
case?).
Each change has a cost:

1. cost of implementing
2. cost of review & rework in review
3. cost of making the code harder to understand for further changes
4. cost of maintenance if the code is reworked, this feature needs to be kept 
working, thus slowing down new features

We *have* to offset this against the value of a feature, especially in a 
bikeshed coloring tool like clang-format.

> Having come from almost nothing to understanding a certain amount of of the 
> code recently in a relatively short space of time (enough to at least 
> contribute a few bug fixes), I'm confident that the "up to speed" costs are 
> actually currently relatively low, and as the Format Library is only 20,000 
> lines and Format.cpp being only ~2500, I don't consider that to be large 
> enough for the code to have come intractable already.

I am working on a patch (for over a year now :( to generalize macro 
configuration instead of having special cases that don't really work well, and 
it's much harder than it should be, as the architecture is getting in my way.

> Of course I understand when someone starts asking for an Option e.g.
> 
> `AllowSpaceAfterSecondCommaOfArgumentListButNotBeforeAnyOfTheOthers: true`
> 
> We may have gone too far ;-), but I almost think that perhaps those cases 
> might be more limited than we think in the end.
> 
> From my recent experiences one nod to complexity was from the C++ language 
> itself.. when trying to fix a bug in `isFunctionDeclarationName()` e,g,
> 
>   A foo(abc);
> 
> 
> does the mean a function foo() taking an argument abc and returning A or an 
> instance of class A called foo being constructed with an argument abc.. This 
> can have all the difference if you want to put a `BraceAfterReturnType`
> 
> The "cost" as you describe it here, over our ability to reason about the code 
> in my view is more likely to come from having to analyze snippets of 
> languages without the full parser to reason with.

Sure, that's where large part of the system complexity comes from, as tackling 
this is not easy.




Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+ 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

@lebedev.ri Are we talking about a general ideology of the long term cost to 
allow any new things in? or to not allow things in this specific case?

because in this specific case all the changes are based on what is really a 
single clause that was already there before, namely

  Style.SpaceBeforeParens == FormatStyle::SBPO_Always

Now that clause is abstracted away to be (the abstraction in my view being a 
nice part of this change, becuase it avoided duplication and made it easier to 
reason about!)

  Style.SpaceBeforeParens == FormatStyle::SBPO_Always  ||
  (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses && 
Right.ParameterCount > 0);

i.e. always add a space, or add a space only when the number of parameters is > 
0 and I don't want a space on empty parameters. (simple enough!)

So whilst I understand the arguments of code complexity, I feel it is sometimes 
used to discourage, what is in at least my view seemingly reasonable requests. 
(not my choice but reasonable all the same)

This was why I asked what was meant by "cost", but you raise the other cost, 
and that is one that is more legitimate... or is it?. have we seen it here as a 
problem or do we just fear it?

Having come from almost nothing to understanding a certain amount of of the 
code recently in a relatively short space of time (enough to at least 
contribute a few bug fixes), I'm confident that the "up to speed" costs are 
actually currently relatively low, and as the Format Library is only 20,000 
lines and Format.cpp being only ~2500, I don't consider that to be large enough 
for the code to have come intractable already.

Of course I understand when someone starts asking for an Option e.g.

`AllowSpaceAfterSecondCommaOfArgumentListButNotBeforeAnyOfTheOthers: true`

We may have gone too far ;-), but I almost think that perhaps those cases might 
be more limited than we think in the end.

From my recent experiences one nod to complexity was from the C++ language 
itself.. when trying to fix a bug in `isFunctionDeclarationName()` e,g,

  A foo(abc);

does the mean a function foo() taking an argument abc and returning A or an 
instance of class A called foo being constructed with an argument abc.. This 
can have all the difference if you want to put a `BraceAfterReturnType`

The "cost" as you describe it here, over our ability to reason about the code 
in my view is more likely to come from having to analyze snippets of languages 
without the full parser to reason with.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D55170#1436732 , @MyDeveloperDay 
wrote:

> > The cost is financial, as it's developer time, which costs real money to 
> > companies. In the end, to support this, people like myself who are doing 
> > this as part of their job spend time that they'd otherwise spend to make 
> > other things better that might be more important.
>
> Don't get me wrong I totally appreciate what you do,
>
> But what you mean is it costs your employer.  That I understand, but not all 
> of us are doing this on behalf of a company (more specially not yours), so 
> you could also say that your employer benefits the other way from those 
> contributors who give their time without them having to spend a dime.


This is not a good argument.
It only considers the one-time cost of adding one particular thing.
It does not consider the cost it incurs on the codebase.

There are likely always ways to implement the same external side-effects
in a way that has near-zero new cost, and ways to implement it in a way
that completely shatters any hope for any further changes.
It is vitally important to avoid the latter.

> I guess my question would be, should the cost to your employer really come 
> into the decision about whether something goes in or not? Other than of 
> course we are totally grateful to them for giving us so much of your time, 
> but that shouldn't have impact on what is worthy to go in should it? or am I 
> wrong?




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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191560.
reuk added a comment.

Removed unnecessary parens.


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

https://reviews.llvm.org/D55170

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine , const FormatToken ,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken ) const;
+
   bool spaceRequiredBetween(const AnnotatedLine , const FormatToken ,
 const FormatToken );
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2403,6 +2403,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken ) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2549,9 +2555,9 @@
  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
tok::kw_new, tok::kw_delete) &&
   (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
+   (spaceRequiredBeforeParens(Right) &&
 (Left.is(tok::identifier) || 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> The cost is financial, as it's developer time, which costs real money to 
> companies. In the end, to support this, people like myself who are doing this 
> as part of their job spend time that they'd otherwise spend to make other 
> things better that might be more important.

Don't get me wrong I totally appreciate what you do,

But what you mean is it costs your employer.  That I understand, but not all of 
us are doing this on behalf of a company (more specially not yours), so you 
could also say that your employer benefits the other way from those 
contributors who give their time without them having to spend a dime.

I guess my question would be, should the cost to your employer really come into 
the decision about whether something goes in or not? Other than of course we 
are totally grateful to them for giving us so much of your time, but that 
shouldn't have impact on what is worthy to go in should it? or am I wrong?




Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

klimek wrote:
> reuk wrote:
> > klimek wrote:
> > > I'm really confused about the changes os parens. It seems like this 
> > > should just change the spaceRequiredBeforeParens(...), but instead seems 
> > > to change parens in a way that completely changes the logic? (or 
> > > alternatively is phabricator showing this wrong?)
> > I think this looks like a bigger change than it really is because I added 
> > an open-parens on line 2548, which then affected the formatting of the 
> > following lines. I also added the `isSimpleTypeSpecifier` check, so that 
> > functional-style casts (`int (foo)`) are given the correct spacing.
> a) why did you add the one in 2548? you didn't change any of the logic, right?
> b) there are 3 extra closing parens in 2560, I see one more new opening in 
> 2556, but that also seems superfluous?
> One question is whether we should pull this apart into bools with names that 
> make sense :)
Isn't this just a classic example of where rewriting this as a series of static 
functions  e.g.

```
static bool someBehavior(Line, Left)
{
 if  (Line.Type == LT_ObjCDecl)
  return true;
 if  (Left.is(tok::semi)
  return true;
..

return false; 
}
```

would be so much easier to understand?


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

reuk wrote:
> klimek wrote:
> > I'm really confused about the changes os parens. It seems like this should 
> > just change the spaceRequiredBeforeParens(...), but instead seems to change 
> > parens in a way that completely changes the logic? (or alternatively is 
> > phabricator showing this wrong?)
> I think this looks like a bigger change than it really is because I added an 
> open-parens on line 2548, which then affected the formatting of the following 
> lines. I also added the `isSimpleTypeSpecifier` check, so that 
> functional-style casts (`int (foo)`) are given the correct spacing.
a) why did you add the one in 2548? you didn't change any of the logic, right?
b) there are 3 extra closing parens in 2560, I see one more new opening in 
2556, but that also seems superfluous?
One question is whether we should pull this apart into bools with names that 
make sense :)


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk marked an inline comment as done.
reuk added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

klimek wrote:
> I'm really confused about the changes os parens. It seems like this should 
> just change the spaceRequiredBeforeParens(...), but instead seems to change 
> parens in a way that completely changes the logic? (or alternatively is 
> phabricator showing this wrong?)
I think this looks like a bigger change than it really is because I added an 
open-parens on line 2548, which then affected the formatting of the following 
lines. I also added the `isSimpleTypeSpecifier` check, so that functional-style 
casts (`int (foo)`) are given the correct spacing.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Mark Zeren via Phabricator via cfe-commits]

mzeren-vmw added a comment.

In D55170#1436087 , @klimek wrote:

> Oh wow, wasn't aware how popular this is. Sigh. I'd like us to push them to 
> change their style guide, but I guess that's not going to fly.


IDK. If the criteria is "public coding standard for widely used project", 
supporting all such things may be unsustainable. Some projects may need to fork 
or find another formatter. I speak as someone that maintains an internal fork 
with features that I do not deem worthy of foisting on trunk.
(And BTW @MyDeveloperDay, whoever you are, Thanks for all of your recent 
activity!)


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1432929 , @MyDeveloperDay 
wrote:

> > I don't think risk is what matters - in the end it's about cost, and cost 
> > is a very technical reason
>
> I'm really sorry I'm not trying to be difficult its just over the years I 
> keep seeing this being said in reviews? but what is the cost? I assume you 
> don't mean a financial cost? could you define cost as a problem so we can 
> address it? what would it take to make `cost` not an issue?


The cost is financial, as it's developer time, which costs real money to 
companies. In the end, to support this, people like myself who are doing this 
as part of their job spend time that they'd otherwise spend to make other 
things better that might be more important. To make cost not an issue we'd need 
to change clang-format to make changes like this significantly simpler to 
review & maintain. I'll add a code comment in a moment, given that this is 
probably something that we'll allow given it fulfills the criteria we've set 
ourselves.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.



Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
-   tok::kw_new, tok::kw_delete) &&
-  (!Left.Previous || Left.Previous->isNot(tok::period) ||
-   (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
-(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
-  Left.MatchingParen->is(TT_LambdaLSquare))) &&
-Line.Type != LT_PreprocessorDirective);
+((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, 
tok::kw_while,
+   tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+   TT_ObjCForIn) ||
+  Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+  (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,

I'm really confused about the changes os parens. It seems like this should just 
change the spaceRequiredBeforeParens(...), but instead seems to change parens 
in a way that completely changes the logic? (or alternatively is phabricator 
showing this wrong?)


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1432832 , @reuk wrote:

> @klimek I agree that the rule is somewhat arbitrary. However, it's the style 
> rule of an established codebase with many users (I don't have a concrete 
> number, but the project has 1400 stars on github 
> ). I've found this patch useful when 
> contributing to JUCE and I thought others might too.


Oh wow, wasn't aware how popular this is. Sigh. I'd like us to push them to 
change their style guide, but I guess that's not going to fly.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

LGTM

(landed here too https://github.com/mydeveloperday/clang-experimental/releases)


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.



> I don't think risk is what matters - in the end it's about cost, and cost is 
> a very technical reason

I'm really sorry I'm not trying to be difficult its just over the years I keep 
seeing this being said in reviews? but what is the cost? I assume you don't 
mean a financial cost? could you define cost as a problem so we can address it? 
what would it take to make `cost` not an issue?


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

@klimek I agree that the rule is somewhat arbitrary. However, it's the style 
rule of an established codebase with many users (I don't have a concrete 
number, but the project has 1400 stars on github 
). I've found this patch useful when 
contributing to JUCE and I thought others might too.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1431854 , @MyDeveloperDay 
wrote:

> > I'm sorry for not having a positive answer here, but I'm not in favor of 
> > this change. The style rule looks like it introduces arbitrary complexity 
> > at a point where I don't understand at all  how it matters. We cannot 
> > possibly support all style guides that come up with random variance. I do 
> > not think this option pulls its weight.
>
> So is this a personal opinion or based on a technical reason (other than the 
> normal mantra about risk & cost),


I don't think risk is what matters - in the end it's about cost, and cost is a 
very technical reason.

>   Could we give @reuk some positive feedback about what he needs to change in 
> this review to get this accepted? He has the other boxes checked about public 
> style etc..
>
> 
> It may not be your preference but its not without some merit for those of us 
> who like to fine tune our style, if that can be done with minimal impact to 
> other styles and is well covered by tests, then can we at least have some 
> discussion first

I'm fine having a discussion - my main question is why this matters. This seems 
a lot more arbitrary than other things we have.

> @reuk please lets continue to fix this revision so it builds as I want to 
> pull it into
> 
> https://github.com/mydeveloperday/clang-experimental
> 
> As an aside, Wouldn't it be great if JUCE was a public style so you could 
> just say
> 
> BasedOnStyle: JUCE
> 
> Where that style defines what JUCE uses? I've always found this odd, I've 
> never understood why we only have Google,WebKit,Mozilla,LLVM, I would have 
> thought it would have been nice to have other big guns Microsoft,Qt generally 
> its only going to be a single function setting the defaults.
> 
> I actually think a change like that would really help people so they don't 
> have to work out all the individual styles, it would help keep .clang-format 
> files small and neat and not full of a bunch of different options.
> 
> Even if JUCE was based on another style say "Google" the code should really 
> be saying
> 
> if (Style.isGoogleDerivedStyle())
> 
>   ..
>
> 
> and not
> 
> Style == FormatStyle::Google
> 
> When its specific to JUCE it would say
> 
> if (Style.isJUCEDerivedStyle())
> 
>   ..
>
> 
> I know when using LLVM, that being able to have a .clang-format file that 
> just says
> 
> BasedOnStyle: LLVM
> 
> is really nice, I don't have to go work out what the LLVM style is, I just 
> get what the project defines, I think it would be great that anything that 
> passes the public style test for submissions should really be able to add 
> that kind of configuration.




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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191050.

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

https://reviews.llvm.org/D55170

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine , const FormatToken ,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken ) const;
+
   bool spaceRequiredBetween(const AnnotatedLine , const FormatToken ,
 const FormatToken );
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken ) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, tok::kw_case, TT_ForEachMacro,
-  TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: include/clang/Format/Format.h:1578
   /// \endcode
   bool SpaceBeforeCpp11BracedList;
 

boolean here not enum see comment below



Comment at: lib/Format/TokenAnnotator.cpp:2608
+(Style.SpaceBeforeCpp11BracedList == FormatStyle::SBBLO_Always ||
+ (Style.SpaceBeforeCpp11BracedList == FormatStyle::SBBLO_NonEmpty &&
+  Right.ParameterCount > 0)))

Did the patch upload correctly? this is still not showing a bool for 
SpaceBeforeCpp11BrackedList


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 191015.
reuk added a comment.

@MyDeveloperDay I'm sorry, you're absolutely correct. I'd got some other 
JUCE-related changes mixed up in this PR. Should be fixed now.


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

https://reviews.llvm.org/D55170

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9337,6 +9337,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11121,6 +11175,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine , const FormatToken ,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken ) const;
+
   bool spaceRequiredBetween(const AnnotatedLine , const FormatToken ,
 const FormatToken );
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken ) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
-(Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
-  tok::kw_switch, 

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

> I'm sorry for not having a positive answer here, but I'm not in favor of this 
> change. The style rule looks like it introduces arbitrary complexity at a 
> point where I don't understand at all  how it matters. We cannot possibly 
> support all style guides that come up with random variance. I do not think 
> this option pulls its weight.

So is this a personal opinion or based on a technical reason (other than the 
normal mantra about risk & cost),  Could we give @reuk some positive feedback 
about what he needs to change in this review to get this accepted? He has the 
other boxes checked about public style etc..

It may not be your preference but its not without some merit for those of us 
who like to fine tune our style, if that can be done with minimal impact to 
other styles and is well covered by tests, then can we at least have some 
discussion first

@reuk please lets continue to fix this revision so it builds as I want to pull 
it into

https://github.com/mydeveloperday/clang-experimental

As an aside, Wouldn't it be great if JUCE was a public style so you could just 
say

BasedOnStyle: JUCE

Where that style defines what JUCE uses? I've always found this odd, I've never 
understood why we only have Google,WebKit,Mozilla,LLVM, I would have thought it 
would have been nice to have other big guns Microsoft,Qt generally its only 
going to be a single function setting the defaults.

I actually think a change like that would really help people so they don't have 
to work out all the individual styles, it would help keep .clang-format files 
small and neat and not full of a bunch of different options.

Even if JUCE was based on another style say "Google" the code should really be 
saying

if (Style.isGoogleDerivedStyle())

  ..

and not

Style == FormatStyle::Google

When its specific to JUCE it would say

if (Style.isJUCEDerivedStyle())

  ..

I know when using LLVM, that being able to have a .clang-format file that just 
says

BasedOnStyle: LLVM

is really nice, I don't have to go work out what the LLVM style is, I just get 
what the project defines, I think it would be great that anything that passes 
the public style test for submissions should really be able to add that kind of 
configuration.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

I'm happy to be wrong, but In current master, SpaceBeforeCpp11BracedList is a 
boolean not an enum  (but I think I've seen a review changing it somewhere 
which maybe isn't landed)

https://reviews.llvm.org/source/llvm-github/browse/master/clang/include/clang/Format/Format.h$1578


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

@MyDeveloperDay I'm not sure you built this branch. Perhaps you applied this 
patch to an older version of the repo. For me, 
SpaceBeforeCpp11BracedListOptions is defined at Format.h:1570.

This builds fine for me, on macOS 10.14 with Xcode 10.1's clang. I just rebased 
onto the most recent clang master and updated llvm. I've been testing/building 
with this command:

  cmake --build build --target FormatTests && 
./build/tools/clang/unittests/Format/FormatTests && cmake --build build 
--target clang-format

@klimek That's disappointing, I thought other JUCE users might benefit from 
this patch (it's quite an established library), and the amount of code that I'm 
a adding is not that high.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.

Pulling this revision down to build it caused build issues with missing 
SBBLO_Always, could you investigate before committing?




Comment at: lib/Format/TokenAnnotator.cpp:2607
+!Left.opensScope() &&
+(Style.SpaceBeforeCpp11BracedList == FormatStyle::SBBLO_Always ||
+ (Style.SpaceBeforeCpp11BracedList == FormatStyle::SBBLO_NonEmpty &&

Where are these SBBLO defined? I cannot find them in Format.h from what I can 
SpaceBeforeCpp11BracedList  is a bool


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1423941 , @MyDeveloperDay 
wrote:

> In D55170#1423798 , @reuk wrote:
>
> > Thanks for the review, and for approving this PR. It's very much 
> > appreciated!
> >
> > I don't have merge rights - could someone commit this for me please?
>
>
> I'd actively encourage you to go get commit access, I think its much better 
> when the commit comes from the author.
>
> There just isn't enough people actively involved in clang-format (and other 
> tools) for us to get reviews even looked at (even for defects), we need more 
> people involved fixing defects and doing reviews, getting commit access shows 
> an intent to fix anything in that comes from your own review which is one of 
> the things the code owners push as being an objection to adding more code.


I'm sorry for not having a positive answer here, but I'm not in favor of this 
change. The style rule looks like it introduces arbitrary complexity at a point 
where I don't understand at all  how it matters. We cannot possibly support all 
style guides that come up with random variance. I do not think this option 
pulls its weight.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D55170#1423798 , @reuk wrote:

> Thanks for the review, and for approving this PR. It's very much appreciated!
>
> I don't have merge rights - could someone commit this for me please?


I'd actively encourage you to go get commit access, I think its much better 
when the commit comes from the author.

There just isn't enough people actively involved in clang-format (and other 
tools) for us to get reviews even looked at (even for defects), we need more 
people involved fixing defects and doing reviews, getting commit access shows 
an intent to fix anything in that comes from your own review which is one of 
the things the code owners push as being an objection to adding more code.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a comment.

Thanks for the review, and for approving this PR. It's very much appreciated!

I don't have merge rights - could someone commit this for me please?


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay accepted this revision.
MyDeveloperDay added a comment.
This revision is now accepted and ready to land.

I'm not the code owner but this LGTM,

Assuming the unit tests remain passing, I'd like to see more items like this in 
clang-format addressed, at present we seem to lack getting even bug fixes 
reviewed!

Thank you for breaking out the common code, I do think more functions like this 
in clang-format would help make the code more readable. I also think the 
various if clauses having comment helps break up these huge monolithic if 
statements

Thanks for the patch.


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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk updated this revision to Diff 189942.
reuk added a comment.

I've rebased onto master, and removed unrelated formatting changes. I've also 
tried to remove some of the duplicate parens-related expressions.

I agree that the heavy nested boolean expressions are a bit painful to read, 
but I'm not sure whether a more general tidy-up falls within the scope of this 
PR.


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

https://reviews.llvm.org/D55170

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9296,6 +9296,60 @@
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
   verifyFormat("auto lambda = [] () { return 0; };", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
+  verifyFormat("auto lambda = []() { return 0; };", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11080,6 +11134,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.h
===
--- lib/Format/TokenAnnotator.h
+++ lib/Format/TokenAnnotator.h
@@ -164,6 +164,8 @@
   unsigned splitPenalty(const AnnotatedLine , const FormatToken ,
 bool InFunctionDecl);
 
+  bool spaceRequiredBeforeParens(const FormatToken ) const;
+
   bool spaceRequiredBetween(const AnnotatedLine , const FormatToken ,
 const FormatToken );
 
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -2400,6 +2400,12 @@
   return 3;
 }
 
+bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken ) const {
+  return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
+ (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+  Right.ParameterCount > 0);
+}
+
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine ,
   const FormatToken ,
   const FormatToken ) {
@@ -2539,19 +2545,19 @@
   return true;
 return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

sorry to be critical, just trying to help get traction on your review...




Comment at: lib/Format/TokenAnnotator.cpp:65
 
-const FormatToken  = *CurrentToken->Previous;  // The '<'.
+const FormatToken  = *CurrentToken->Previous; // The '<'.
 if (Previous.Previous) {

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:526
 if (!Contexts.back().FirstObjCSelectorName) {
-FormatToken* Previous = CurrentToken->getPreviousNonComment();
-if (Previous && Previous->is(TT_SelectorName)) {
-  Previous->ObjCSelectorNameParts = 1;
-  Contexts.back().FirstObjCSelectorName = Previous;
-}
+  FormatToken *Previous = CurrentToken->getPreviousNonComment();
+  if (Previous && Previous->is(TT_SelectorName)) {

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:1392
+   // FIXME(bug 36976): ObjC return types shouldn't use
+   // TT_CastRParen.
Current.Previous && Current.Previous->is(TT_CastRParen) &&

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2420
+Right.MatchingParen->Next &&
+Right.MatchingParen->Next->is(tok::colon);
 return !IsLightweightGeneric && Style.ObjCSpaceBeforeProtocolList;

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2545
+  Left.Tok.getIdentifierInfo()->isKeyword(
+  getFormattingLangOpts(Style &&
+Line.Type != LT_PreprocessorDirective) ||

would be nice to pull the 3 items into a function because you repeat it on line 
2551



Comment at: lib/Format/TokenAnnotator.cpp:2548
+   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+Right.ParameterCount >= 1 &&
+(Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||

something similar here when combined with line 2540, looks a lot like the 2 
patterns below?

almost like its:


```
bool allowSpaceBeforeIfFunctionHasParameters(Style,Right);
```



Comment at: lib/Format/TokenAnnotator.cpp:2617
+if (Left.MatchingParen &&
+Left.MatchingParen->is(TT_ProtoExtensionLSquare) &&
 Right.isOneOf(tok::l_brace, tok::less))

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass



Comment at: lib/Format/TokenAnnotator.cpp:2833
+   (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
+Right.ParameterCount >= 1);
   if (Right.is(TT_TemplateOpener) && Left.is(tok::r_paren) &&

can this and the above section on line 2745 be combined? I'm not sure if the 
order of the "if" indicates precedence



Comment at: lib/Format/TokenAnnotator.cpp:3138
 if (Right.is(Keywords.kw_is)) {
-  const FormatToken* Next = Right.getNextNonComment();
+  const FormatToken *Next = Right.getNextNonComment();
   // If `is` is followed by a colon, it's likely that it's a dict key, so

unrelated change, consider backing out, perhaps we could do a clang-format of 
the file as a separate pass


Repository:
  rC Clang

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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Hi @reuk,

I'm trying to go over old reviews and see if they are still desired, I stumbled 
on your review and want to see if its still important. Here is some initial 
feedback.

I know its ironic that the clang-format code isn't clang-formatted itself, but 
it might help if your changes didn't also include additional unrelated 
clang-formatting changes because its a little harder to see what actually 
changed, and your review would be much smaller.

perhaps the next steps could be:

1. you rebase to current head (since they've left you hanging her for 3 months!)
2. you use something like git difftool to interactively put back the 
"formatting" of code which isn't part of this change (yes we should really 
clang-format all of clang-format, because I know I've hit this myself recently)
3. The one thing I struggle with when looking at the clang-format code and this 
review (and its probably  part of why the owners seem a bit reluctant to let 
anything else in), is encapsulated by this part of the change



  return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
 (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
  (Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
tok::kw_switch, tok::kw_case, TT_ForEachMacro,
TT_ObjCForIn) ||
   Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
   (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
 tok::kw_new, tok::kw_delete) &&
(!Left.Previous || Left.Previous->isNot(tok::period) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
  Right.ParameterCount >= 1 &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_square) || Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective);

There is nothing wrong with it per say I'm sure it works beautifully but its a 
little hard to reason about.

I know you only adding to the end of it, but I think if this was in clang-tidy 
we'd be asked to write it as a function/matcher to encapsulate this kind of 
functionality. Its also an indictment of clang format that this isn't aligning 
in such a way that it can be more easily read, I mean I know its done its job, 
but its quite a challenge to look at! ;-)

To me clang-format needs more functions like

  Left.isIdentifier()
  Left.isParen()
  Left.isLSquare() 
  Right.hasParameters()

in order to help break complex conditional like this down into a readable form 
which can be rationalised about.

This would help hide some of the tok::XXX complexity which can make my eyes at 
least go funny.. and code like this (which I think you added)

  Left.Tok.getIdentifierInfo() && 
Left.Tok.getIdentifierInfo()->isKeyword(getFormattingLangOpts(Style)))   
(especially as its repeated in the clause)

It looks like it means some means something functional which perhaps could be 
combined:

(sorry I don't know what its doing but I think you do)

  isLanguageKeyWord(Left,Style) 

or

  Left.isLanguageKeyWord(Style);

If you could add the part you added, is a more succinct way,

  || isFunctionWithNoArguments(Left,Right)

rather than

  ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective) ||
 (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses 
&&
  Right.ParameterCount >= 1 &&
  (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
   Left.is(tok::r_square) || Left.is(tok::r_paren) ||
   (Left.Tok.getIdentifierInfo() &&
Left.Tok.getIdentifierInfo()->isKeyword(
getFormattingLangOpts(Style &&
  Line.Type != LT_PreprocessorDirective);

I think there would be a stronger argument that it should go in, given that you 
have a publicly available style guide etc... (owners may not agree!)

Perhaps too much of my own opinion here... but maybe the "development costs" 
would go down if thing like

  Left.endsSequence(tok::kw_constexpr, tok::kw_if)

where written as

  Left.isConstExprIf()

if that is what its doing?


Repository:
  rC Clang

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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk added a subscriber: klimek.
reuk added a comment.

Would someone review this please? I'm not sure who to add for review (sorry), 
maybe one of the following?
@klimek @Typz @krasimir


Repository:
  rC Clang

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

https://reviews.llvm.org/D55170



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

[PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option

[Reuben Thomas via Phabricator via cfe-commits]

reuk created this revision.
reuk added reviewers: Typz, krasimir, cfe-commits.

This patch aims to add support for the following rules from the JUCE coding 
standards:

- Always put a space before an open parenthesis that contains text - e.g. foo 
(123);
- Never put a space before an empty pair of open/close parenthesis - e.g. foo();

The entire set of JUCE coding guidelines can be found here 
. Unfortunately, 
clang-format can't enforce all of these style rules at the moment, so I'm 
trying to add support for them.

Patch by Reuben Thomas


Repository:
  rC Clang

https://reviews.llvm.org/D55170

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

Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -9271,6 +9271,59 @@
   verifyFormat("typedef void (*cb) (int);", Space);
   verifyFormat("T A::operator() ();", Space);
   verifyFormat("X A::operator++ (T);", Space);
+  verifyFormat("int x = int (y);", Space);
+
+  FormatStyle SomeSpace = getLLVMStyle();
+  SomeSpace.SpaceBeforeParens = FormatStyle::SBPO_NonEmptyParentheses;
+
+  verifyFormat("[]() -> float {}", SomeSpace);
+  verifyFormat("[] (auto foo) {}", SomeSpace);
+  verifyFormat("[foo]() -> int {}", SomeSpace);
+  verifyFormat("int f();", SomeSpace);
+  verifyFormat("void f (int a, T b) {\n"
+   "  while (true)\n"
+   "continue;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("if (true)\n"
+   "  f();\n"
+   "else if (true)\n"
+   "  f();",
+   SomeSpace);
+  verifyFormat("do {\n"
+   "  do_something();\n"
+   "} while (something());",
+   SomeSpace);
+  verifyFormat("switch (x) {\n"
+   "default:\n"
+   "  break;\n"
+   "}",
+   SomeSpace);
+  verifyFormat("A::A() : a (1) {}", SomeSpace);
+  verifyFormat("void f() __attribute__ ((asdf));", SomeSpace);
+  verifyFormat("*( + 1);\n"
+   "&(()[1]);\n"
+   "a[(b + c) * d];\n"
+   "(((a + 1) * 2) + 3) * 4;",
+   SomeSpace);
+  verifyFormat("#define A(x) x", SomeSpace);
+  verifyFormat("#define A (x) x", SomeSpace);
+  verifyFormat("#if defined(x)\n"
+   "#endif",
+   SomeSpace);
+  verifyFormat("auto i = std::make_unique (5);", SomeSpace);
+  verifyFormat("size_t x = sizeof (x);", SomeSpace);
+  verifyFormat("auto f (int x) -> decltype (x);", SomeSpace);
+  verifyFormat("int f (T x) noexcept (x.create());", SomeSpace);
+  verifyFormat("alignas (128) char a[128];", SomeSpace);
+  verifyFormat("size_t x = alignof (MyType);", SomeSpace);
+  verifyFormat("static_assert (sizeof (char) == 1, \"Impossible!\");",
+   SomeSpace);
+  verifyFormat("int f() throw (Deprecated);", SomeSpace);
+  verifyFormat("typedef void (*cb) (int);", SomeSpace);
+  verifyFormat("T A::operator()();", SomeSpace);
+  verifyFormat("X A::operator++ (T);", SomeSpace);
+  verifyFormat("int x = int (y);", SomeSpace);
 }
 
 TEST_F(FormatTest, ConfigurableSpacesInParentheses) {
@@ -11055,6 +11108,8 @@
   FormatStyle::SBPO_Always);
   CHECK_PARSE("SpaceBeforeParens: ControlStatements", SpaceBeforeParens,
   FormatStyle::SBPO_ControlStatements);
+  CHECK_PARSE("SpaceBeforeParens: NonEmptyParentheses", SpaceBeforeParens,
+  FormatStyle::SBPO_NonEmptyParentheses);
   // For backward compatibility:
   CHECK_PARSE("SpaceAfterControlStatementKeyword: false", SpaceBeforeParens,
   FormatStyle::SBPO_Never);
Index: lib/Format/TokenAnnotator.cpp
===
--- lib/Format/TokenAnnotator.cpp
+++ lib/Format/TokenAnnotator.cpp
@@ -62,7 +62,7 @@
 if (NonTemplateLess.count(CurrentToken->Previous))
   return false;
 
-const FormatToken  = *CurrentToken->Previous;  // The '<'.
+const FormatToken  = *CurrentToken->Previous; // The '<'.
 if (Previous.Previous) {
   if (Previous.Previous->Tok.isLiteral())
 return false;
@@ -366,7 +366,7 @@
   // specifier parameter, although this is technically valid:
   // [[foo(:)]]
   if (AttrTok->is(tok::colon) ||
-  AttrTok->startsSequence(tok::identifier, tok::identifier) || 
+  AttrTok->startsSequence(tok::identifier, tok::identifier) ||
   AttrTok->startsSequence(tok::r_paren, tok::identifier))
 return false;
   if (AttrTok->is(tok::ellipsis))
@@ -523,11 +523,11 @@
 // Here, we set FirstObjCSelectorName when the end of the method call is
 // reached, in case it was not set already.
 if (!Contexts.back().FirstObjCSelectorName) {
-FormatToken* Previous = CurrentToken->getPreviousNonComment();
-