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

hel-ableton added a comment. Hi Owen, thanks for reaching out, no. In fact we realized that we don't have enough capacity to be working on it. Best, Henrik - Henrik Lafrenz, Ableton AG Schoenhauser Allee 6-7, 10119 Berlin, Germany T: +49 30 288 763-256 Board: Gerhard Behles, Jan Bohl Supervis

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

owenpan added a comment. Herald added a reviewer: MyDeveloperDay. @hel-ableton do you intend to continue working on this? If not, we can commandeer it and finish or abandon it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 _

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

owenpan added a comment. In D136154#3882163 , @hel-ableton wrote: > I hope we can somewhat start from square 1 again with this. See https://reviews.llvm.org/D136154#3890747. It doesn't "fix" the last example in https://github.com/llvm/llvm-project/issu

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

owenpan added inline comments. Comment at: clang/lib/Format/ContinuationIndenter.cpp:803-804 CurrentState.LastSpace = State.Column; - } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr, - TT_CtorInitializerColon)) &&

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

owenpan added inline comments. Comment at: clang/lib/Format/ContinuationIndenter.cpp:803-808 - } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr, - TT_CtorInitializerColon)) && +} else if (Previous.is(TT_CtorInitializerColon)) {

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

hel-ableton marked an inline comment as done. hel-ableton added a comment. I filed an issue now in the llvm repo, it just describes how the formatting in our case changes, and what _might_ be done to keep it from doing so. I have consciously left out any proposed unit tests, because at this poin

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

hel-ableton marked an inline comment as done. hel-ableton added a comment. > After you write a New Inline Comment, click the Save Draft button. Then click > the Submit button near the bottom of the screen. Makes sense, only I see no "Save Draft" button... Comment at: clang/li

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

owenpan added a comment. In D136154#3867328 , @MyDeveloperDay wrote: > Can we log a GitHub issue I can’t see what you are trying to fix @hel-ableton Can you log an issue for this and add a link to it in `SUMMARY`? In D136154#3867441

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

MyDeveloperDay added a comment. You can remove this line in my view... > Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) { CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 ___ cfe

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

hel-ableton added a comment. I'm not sure I'm following where you're getting at. So far I'm getting the following: - my proposed fix was not ideal, and only "accidentally" fixed our issue - the fix including `Previous.isOneOf(TT_BinaryOperator...` is a better fix - we should write a proper test

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

HazardyKnusperkeks added a comment. In D136154#3868036 , @hel-ableton wrote: > In D136154#3867935 , > @MyDeveloperDay wrote: > >> You've dropped the test on your most recent updated > > Oh, that's why it appeare

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

MyDeveloperDay added a comment. Going back to the original bug that caused this.. if you leave in your BOS_NonAssignment I think you'll reintroduce the original bug.. which possibly should have been called "incorrectly adds extra continuation indent spaces with BreakBeforeBinaryOperators set

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

MyDeveloperDay added a comment. Looking at the documentation, I think BreakBeforeBinaryOperators is unrelated to your test case. I would remove any references of it from your fix. F24968180: image.png CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1

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

MyDeveloperDay added inline comments. Comment at: clang/lib/Format/ContinuationIndenter.cpp:814 +if (Style.BreakBeforeBinaryOperators == FormatStyle::BOS_None || +Style.BreakBeforeBinaryOperators == FormatStyle::BOS_NonAssignment) { CurrentState.LastSpace = Sta

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

hel-ableton marked an inline comment as done. hel-ableton added a comment. > I do, but I'd like to hear @owenpan and @HazardyKnusperkeks view.. (each of > us is better in different part of clang-format, and I value their opinion!) Fair enough. CHANGES SINCE LAST ACTION https://reviews.llvm.o

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

hel-ableton updated this revision to Diff 468907. hel-ableton added a comment. Fixed missing newline. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 Files: clang/lib/Format/ContinuationIndenter.cpp clang/unittests/Format/FormatTest.cpp

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

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:6615 + " fourthArgWithLongName) {}\n" + "};", Style);} missing newline CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://review

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

MyDeveloperDay added a comment. In D136154#3868151 , @hel-ableton wrote: > If you think this would be the better fix, AFAICS it does what we need. > What's the Beyonce rule, by the way? I do, but I'd like to hear @owenpan and @HazardyKnusperkeks view..

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

MyDeveloperDay added a comment. > Beyonce rule "If they liked it they should have put a test on it" CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 ___ cfe-commits mailing list cfe-commits@lists.llv

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

hel-ableton marked an inline comment as done. hel-ableton added a comment. In D136154#3867968 , @MyDeveloperDay wrote: > Doesn't something like this achieve the same > >CurrentState.Indent = State.Column; >CurrentState.LastSpace = State.

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

hel-ableton updated this revision to Diff 468878. hel-ableton added a comment. Using verifyFormat() now instead of EXPECT_EQ() CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 Files: clang/lib/Format/ContinuationIndenter.cpp clang/unittests

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

hel-ableton added a comment. > 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 unne

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

hel-ableton updated this revision to Diff 468874. hel-ableton added a comment. This should bring back the formerly introduced unit test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 Files: clang/lib/Format/ContinuationIndenter.cpp clang

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

hel-ableton added a comment. In 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

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

MyDeveloperDay added a comment. I think in hindsight D50699: [clang-format] fix PR38525 - Extraneous continuation indent spaces with BreakBeforeBinaryOperators set to All was overly aggressive again the `TT_CtorInitializerColon` case CHANGES SINCE LAST ACTION

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

MyDeveloperDay added a comment. Doesn't something like this achieve the same CurrentState.Indent = State.Column; CurrentState.LastSpace = State.Column; - } else if ((Previous.isOneOf(TT_BinaryOperator, TT_ConditionalExpr, - TT_CtorInitializerColon

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

MyDeveloperDay added a comment. Its probably worth looking at how to fix this more in the context of the original bug, Its something I don't like is where we pile in all these expressions, without any comments about what cases we are handling here.. I'd rather handle each one at a time.. as I do

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

MyDeveloperDay added a comment. I get why this fix works for you, but I don't agree that the fix is the correct one.. for example you don't have any BinaryOperators in that code, lets just say in my style I had BreakBeforeBinaryOperators: All Then your fix wouldn't cover me and I'd get exac

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

MyDeveloperDay added a comment. You've dropped the test on your most recent updated Comment at: clang/unittests/Format/FormatTest.cpp:6603-6622 + EXPECT_EQ( + "struct Derived {\n" + " Derived(\n" + "int firstArgWithLongName,\n" + "int secondArgWith

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

HazardyKnusperkeks added a comment. In D136154#3867461 , @hel-ableton wrote: >> must have been introduced somewhere between 7.1.0 and 10.0.0 > > In fact, someone in our team thought that this must have been introduced > exactly with this commit: > > htt

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

hel-ableton added a comment. Adding two inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:6599-6601 + Style.AlignAfterOpenBracket = FormatStyle::BAS_AlwaysBreak; + Style.BinPackParameters = false; + Style.ContinuationIndentWidth = 2; Hazardy

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

hel-ableton added a comment. > must have been introduced somewhere between 7.1.0 and 10.0.0 In fact, someone in our team thought that this must have been introduced exactly with this commit: https://github.com/llvm/llvm-project/commit/4636debc271f09f730697ab6873137a657c828f9 CHANGES SINCE LAS

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

hel-ableton added a comment. In D136154#3867328 , @MyDeveloperDay wrote: > Can we log a GitHub issue I can’t see what you are trying to fix Without the fix, the arguments to the Base class would be on the same level as the Base class itself. But this a

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

hel-ableton added a comment. In D136154#3867328 , @MyDeveloperDay wrote: > Can we log a GitHub issue I can’t see what you are trying to fix I'm sorry if this is unclear. The background to this is that our repository is currently formatted using clang-f

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

hel-ableton updated this revision to Diff 468793. hel-ableton added a comment. Sorry about the plus sign! I'm fairly unfamiliar with your process, so something between making a diff and making a patch must have gone wrong. About the brackets, I was wondering why there weren't any, but didn't see

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

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. Needs a better title to the review please to explain what it’s doing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1361

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

MyDeveloperDay added a comment. Is it seeing the ‘ :’ as a binary operator? And not an inheritance colon? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 ___ cf

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

MyDeveloperDay added a comment. Can we log a GitHub issue I can’t see what you are trying to fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136154/new/ https://reviews.llvm.org/D136154 ___ cfe-commits