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