[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. I've found yet another bug. When `AllowShortEnumsOnASingleLine` is `true` and a multiline enum is forced (by line length, trailing comma, etc.), multiple names for the enum are placed on separate lines. Example: enum { A, B, C, } ShortEnum1, ShortEnum2; Is

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:3679-3680 +if (remainingFile[whileIndex] != '\n' && +(remainingFile[whileIndex] == ' ' && + remainingFile[whileIndex - 1] == ' ')) { + remainingLineCharCount++;

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 331172. atirit added a comment. Removed `strlen` call and added tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:3683 +Style.BraceWrapping.AfterEnum; +bool isLineTooBig = (strlen(Right.TokenText.data()) + + Right.OriginalColumn) > Style.ColumnLimit; curdeius

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 330860. atirit added a comment. Fixed remote build Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 330738. atirit added a comment. Implemented requested changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:13319 AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman; + AllmanBraceStyle.AllowShortEnumsOnASingleLine = false; HazardyKnusperkeks wrote: > curdeius wrote: > > That

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:3683 +Style.BraceWrapping.AfterEnum; +bool isLineTooBig = (strlen(Right.TokenText.data()) + + Right.OriginalColumn) > Style.ColumnLimit; curdeius

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 330570. atirit added a comment. Attempting to fix build Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. I would like to request that this commit be considered for merging. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 ___ cfe-commits

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 330568. atirit added a comment. Added comments for the previous commit's changes and cleaned up those changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files:

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 330495. atirit added a comment. Fixed `AfterEnum`'s compatibility with `AllowShortEnumsOnASingleLine` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files:

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. In D93938#2614825 , @HazardyKnusperkeks wrote: > If the bugs are (very) similar, I could live with one fix for both. Otherwise > you should fix the other bug first, if its blocking this change. The only thing linking the bugs is

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. In D93938#2612952 , @HazardyKnusperkeks wrote: > In my opinion you should then, either temporarily deactivate the test, or fix > the bug first. A failing test blocks the pipeline and confuses everyone > working on the project.

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. In D93938#2610568 , @curdeius wrote: > I hadn't had another look, because the CI still shows the test `AfterEnum` to > be failing. I know the test fails; it's due to a separate bug wherein `AfterEnum: true` and

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. Has anyone had the chance to review this yet? (not trying to be pushy, just curious) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 ___

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. The test still is there; Git is just showing the diff strangely. I didn't modify that test at all. The corner case bug doesn't affect `FormatTest.ShortEnums` because that test effectively has `AfterEnum: false`. Should I add cases for `AfterEnum: true` in that test too?

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 314301. atirit added a comment. Squashed commits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/UnwrappedLineParser.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. I don't understand; should every commit I've made be squashed into one and then submitted here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. `AfterEnum: true` currently overrides `AllowShortEnumsOnASingleLine: true`. If this is epxected behaviour then I'll modify the test to accomodate that, but otherwise, there's a separate issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 314299. atirit added a comment. Removed multiple enum names from new test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/unittests/Format/FormatTest.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. The first test fails due to the aforementioned corner case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 ___ cfe-commits mailing list

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 314296. atirit added a comment. Added unit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/unittests/Format/FormatTest.cpp Index:

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. In D93938#2476519 , @MyDeveloperDay wrote: > You need to have these conversations by adding new unit tests that prove your > point, I highly doubt I'll personally be willing to accept any revision > without more unit tests than

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. I think accepting a revision that includes only a fix for `AfterEnum` being ignored (not the corner case) and the new unit test would be the best way to go about this, since they're separate bugs. Then I can fix the corner case (and compatibility with the new unit test)

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. OK, I'm getting a few more failed unit tests and I'm really not sure what correct formatting behaviour is anymore, so I'll just ask. Assuming the following settings: BreakBeforeBraces: Custom BraceWrapping: { AfterEnum: true } AllowShortEnumsOnASingleLine:

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. Turns out the `true/true` bug goes quite deep. I've managed to resolve the first bit of it with a hack that I'm sure will warrant some criticism, but I haven't familiarised myself with this codebase enough to write a cleaner version. The second issue I'm still

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. After writing a unit test, I've found that a combination of `AfterEnum: true` and `AllowShortEnumsOnASingleLine: true` doesn't function properly. My next revision will include a fix for that alongside the unit test. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. > This seems to be that in "Attach" mode, then > AllowShortEnumsOnASingleLine=false doesn't attach the brace. That is correct, but the main issue is that `AfterEnum: false`, which Attach mode implies, doesn't function correctly. > Said all that, it *seems* to me that

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. @MyDeveloperDay Would you mind explaining what changes you're wanting? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 ___ cfe-commits

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. Additionally, this bug has already been reported: https://bugs.llvm.org/show_bug.cgi?id=46927 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. @MyDeveloperDay I expect to see exactly that. `clang-format` currently does not respect `AfterEnum`, treating it as always `true`. This is why I changed `UnwrappedLineParser.cpp`. The unit test is incorrect as the style used for the `clang-format` C(++) unit tests uses

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 314162. atirit added a comment. Changed commit username and email Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 Files: clang/lib/Format/UnwrappedLineParser.cpp

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit updated this revision to Diff 314149. atirit added a comment. Fixed the FormatTest.ShortEnums unit test and fixed compatibility with `FormatTestCSharp.CSharpKeyWordEscaping` and `FormatTestJS.EnumDeclarations` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. The `FormatTestJS.EnumDeclarations` test in fact isn't broken, but `FormatTest.ShortEnums` and `FormatTestCSharp.CSharpKeyWordEscaping` are. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/

[PATCH] D93938: [clang-format] Fixed AfterEnum handling

atirit added a comment. From what I can tell the unit tests are broken. Take `FormatTest.ShortEnums` for example. It passes the following code: enum { A, B, C } ShortEnum1, ShortEnum2; And expects no changes to be made. However, format unit tests use the `BreakBeforeBraces:

[PATCH] D93938: Fixed AfterEnum handling

atirit created this revision. atirit requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D93938 Files: clang/lib/Format/UnwrappedLineParser.cpp Index: