[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