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