[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

[Francois Ferrand via Phabricator via cfe-commits]

Typz added inline comments.


================
Comment at: unittests/Format/FormatTest.cpp:2476
       "bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                     + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                     + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                 == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                            * bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
-      "                        + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
-      "             && aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                        * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
-      "                    > ccccccccccccccccccccccccccccccccccccccccc;",
+      "                   + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
+      "                   + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
----------------
djasper wrote:
> Typz wrote:
> > Typz wrote:
> > > djasper wrote:
> > > > This looks very inconsistent to me.
> > > not sure what you mean, I do not really understand how this expression was aligned before the patch...
> > > it is not so much better in this case with the patch, but the '&&' is actually right-aligned with the '=' sign.
> > Seeing the test just before, I see (when breaking after operators) that the operands are actually right-aligned, e.g. all operators are on the same column.
> > 
> > So should it not be the same when breaking before the operator as well (independently from my patch, actually)?
> > 
> >   bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                      + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                      + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                 == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                          * bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
> >                      + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n"
> >             && aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                      * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\n"
> >                  > ccccccccccccccccccccccccccccccccccccccccc;
> > 
> > Not sure I like this right-alignment thing, but at least I start to understand how we get this output (and this may be another option to prefer left-alignment?)
> The right alignment is not relevant. I think I just got "playful" when writing this test.
> 
> What's happening here is that we align the operators of subsequent lines to the previous operand. You are right that this is not intuitive wrt. the option's name, but it is the behavior that we intended and that we had seen in styles that use this.
> 
> Now, what we also do is add another ContinuationIndentWidth to highlight operator precedence. Basically think of this as replacing parentheses that you would put there instead:
> 
>   int x = (aaaaa
>            * bbbbb) // The "*" is intendet a space because of the paren.
>           + ccccc;
> 
>   int x = aaaaa
>               * bbbbb // Instead we use ContinuationIndentWidth here.
>           + ccccc;
> 
> What I mean about this being inconsistent is that now you align the highest-level operands because as you say that's what's expected from the option, but you still don't align other precedences, which seems wrong. If you really wanted to align all operands, that would look like:
> 
>   bool value = aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>              + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>              + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>             == aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>              * bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
>              + bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb
>             && aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>              * aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
>              > ccccccccccccccccccccccccccccccccccccccccc;
> 
> But then, that's really a tough expression to understand. I mean, this is a fabricated example and possibly doesn't happen often in real life, but I am slightly worried about it and the inconsistency here.
So how should we proceed?

As I understand it, there are 2 separate issues here:
 * The current behavior is actually the expected one, and should not be modified
 * The indentation due to 'fake parenthesis' is not very explicit (even with the current implementation)

Changing AlignOperands into an OperandAlignment enum, with 3 options (`OA_NotAligned`, same as `AlignOperands=false`; `OA_AlignOperator` (or OA_Align), same the current `AlignOperands=true`; and `OA_AlignOperands` (or OA_StrictAlign), with my new behavior), would solve the first problem. Would it be acceptable?

I don't think this is related to the extra indentation issue, the problem is already present, and the existing behavior is not that consistent or clear... But I think the 'fake parenthesis' indentation could easily be changed, independently from my patch, to use a dedicated `operatorPrecedenceIndentWith` instead of ContinuationIndentWidth (or a boolean indentOperatorPrecedence) : setting it would allow to force all operators aligned, independenty from the other options (and in a separate patch?).

What do you think?




https://reviews.llvm.org/D32478