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
