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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In D55170#1439864 <https://reviews.llvm.org/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) ||
+              (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
----------------
MyDeveloperDay wrote:
> 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?
Agreed, but I think even a:
bool someBehavior = (A || B) && (C || D);
would help :)

That said, parens look good now.


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

https://reviews.llvm.org/D55170