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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
