djasper added a comment.
It's not about whether or not we like the patch. It's whether adding these
options is a good trade-off for clang-format overall. If we find that actually
more people would find these styles desirable, we can reconsider.
I have left some comments anyway in case you want to keep the patch around.
> Format.h:603
>
> + /// \brief If ``true``, spaces will be inserted around if/for/while
> conditions.
> + bool SpacesAroundConditions;
It's actually more than if/for/while.
> Format.cpp:355
> IO.mapOptional("SpacesInSquareBrackets", Style.SpacesInSquareBrackets);
> + IO.mapOptional("SpacesAfterNot",
> + Style.SpacesAfterNot);
Unnecessary linebreaks.
> TokenAnnotator.cpp:1989
> + if (Left.is(tok::l_paren) && Left.Previous &&
> + Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for,
> tok::kw_while,
> + tok::kw_switch, TT_ForEachMacro))
Indent is off.
> TokenAnnotator.cpp:1989-1990
> + if (Left.is(tok::l_paren) && Left.Previous &&
> + Left.Previous->isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for,
> tok::kw_while,
> + tok::kw_switch, TT_ForEachMacro))
> + return true;
This should go into a helper function or lambda so that it can be reused. What
about catch? Also some of these aren't tested, e.g. the preprocessor ones.
> TokenAnnotator.cpp:1993
> + if (Right.is(tok::r_paren) && Right.MatchingParen &&
> Right.MatchingParen->Previous &&
> + Right.MatchingParen->Previous->isOneOf(tok::kw_if, tok::pp_elif,
> tok::kw_for, tok::kw_while,
> + tok::kw_switch,
> TT_ForEachMacro))
Indent is off.
> TokenAnnotator.cpp:2232
> }
> + if (Style.SpacesAfterNot && Left.is(tok::exclaim))
> + return true;
Note that at least JavaScript/TypeScript has a ! postfix operator, i.e. unary
operator that applies to the token before it. You definitely don't want to
always add a space after that.
> TokenAnnotator.cpp:2233
> + if (Style.SpacesAfterNot && Left.is(tok::exclaim))
> + return true;
> if (Left.is(TT_UnaryOperator))
Indent is off.
> TokenAnnotator.cpp:2252
> return false;
> - if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment))
> + if (Right.is(tok::coloncolon) && !Left.isOneOf(tok::l_brace, tok::comment,
> tok::l_paren))
> return (Left.is(TT_TemplateOpener) &&
Is this related in any way? I don't see a test with ::
> FormatTest.cpp:11508
> + Spaces.SpacesAfterNot = true;
> + verifyFormat("if (! a)\n return;", Spaces);
> + verifyFormat("while (! (x || y))\n return;", Spaces);
While you have added some test here, I think the more debatable ones are
actually where there is also a space before the !, e.g.:
if (a && ! b) ..
return ! c;
bool x = ! y && z;
The last one is especially tricky, because it makes it less clear that ! binds
stronger than &&. Might seem obvious in this case, but IIRC, we fought quite a
few bugs of the form:
if (! a < b) ..
Until a warning was added in Clang.
https://reviews.llvm.org/D25171
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
