[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/

https://reviews.llvm.org/D55170