[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D55170: [clang-format]: Add NonEmptyParentheses spacing option
From:       MyDeveloperDay via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2019-02-28 23:02:45
Message-ID: 52da77b49697a132d152ee2ff6dfae86 () localhost ! localdomain
[Download RAW message or body]

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



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


[prev in list] [next in list] [prev in thread] [next in thread]