[PATCH] D136154: [clang-format] Fix the continuation indenter

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added a comment.

In D136154#3868036 <https://reviews.llvm.org/D136154#3868036>, @hel-ableton wrote:

> In D136154#3867935 <https://reviews.llvm.org/D136154#3867935>, @MyDeveloperDay wrote:
>
>> You've dropped the test on your most recent updated
>
> Oh, that's why it appeared from the diff. Apologies again, I'm really unfamiliar with your process. I guess it puzzles me why it's described in https://llvm.org/docs/Phabricator.html#phabricator-request-review-web that I should make any commits at all, when it's just diffs that I'm supposed to submit. Anyway, I'll try to bring it back.

How you get your diff doesn't matter. I make the real commits (which I will commit, if approved), and then just do `git diff -U9999999 HEAD^1 > file`.

In D136154#3868070 <https://reviews.llvm.org/D136154#3868070>, @hel-ableton wrote:

>> The problem as I see it was that the original bug, highly constrained the cases where "CurrentState.LastSpace = State.Column;" to one particular style (which if it happens to be your style great but not if its not.
>
> You mean the original bugfix was already unnecessarily constrained, and now my proposed fix is only opening it up for one my case? That may be so. In any case this might really not be the ideal fix, and I'm open to any other, better way of fixing it.

I too think one should split the conditions, as good as one can overview them. And add tests for each case. It seems you still have a running clang-format 6, then you are in the perfect place to regression test against that without any additional overhead.

Please mark all comments as done, if they are done.



================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:803
     CurrentState.LastSpace = State.Column;
-  } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr,
-                               TT_CtorInitializerColon)) &&
+    } else if (Previous.is(TT_CtorInitializerColon)) {
+      CurrentState.LastSpace = State.Column;
----------------
Before it was followed with an `&&`, I don't know which one was for this case, and which one for the other, or for both. This is a bit hard, but maybe this is just to few checks?


================
Comment at: clang/unittests/Format/FormatTest.cpp:6599-6601
+  Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak;
+  Style.BinPackParameters = false;
+  Style.ContinuationIndentWidth = 2;
----------------
hel-ableton wrote:
> HazardyKnusperkeks wrote:
> > Do you need this for the observed effect? From the description I don't see that.
> I think it's necessary for the test to work, otherwise the value would fall back to 4, I think? Would you prefer the test to be re-written with the default value, instead? 
At least the indentation width shouldn't matter and changing that here can give the impression it does.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136154/new/

https://reviews.llvm.org/D136154