[PATCH] D25171: clang-format: Add two new formatting options

[Daniel Jasper via cfe-commits]

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