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

MarcusJohnson91 added a comment. Not trying to take over this patch, because I've got a ton of patches I need to be finishing up myself. but I think the problematic code is in UnwrappedLineParser.cpp:1900 if (FormatTok->Tok.getKind() == ClosingBraceKind) { if (IsEnum &&

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

MarcusJohnson91 added a comment. In D93938#2832825 , @Userbla wrote: > I applied this fix locally to a branch based off llvm 11.x and the > `FormatTest.FormatsTypedefEnum` test now fails. I'm running into this bug too. typedef enum Blah { One =

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

MyDeveloperDay added a comment. >> Since you use `== ' '` twice, `remainingLineCharCount` will count only >> consecutive spaces, right? >> But you want to count other characters, no? >> So, IIUC, the condition you want is `rF[wI] != '\n' && !(rF[wI] == ' ' && >> rF[wI - 1] == ' ')` (mind the

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

Userbla added a comment. In D93938#2833045 , @Userbla wrote: > It fails in the second case because we don't respect the 'AfterEnum = true' > and collapse the brace. It appears there is buggy logic in the > `remainingLineCharCount` stuff which others

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

Userbla added a comment. In D93938#2832825 , @Userbla wrote: > I applied this fix locally to a branch based off llvm 11.x and the > `FormatTest.FormatsTypedefEnum` test now fails. So this test is failing: TEST_F(FormatTest, LongEnum) {

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

Userbla added a comment. I applied this fix locally to a branch based off llvm 11.x and the `FormatTest.FormatsTypedefEnum` test now fails. 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

MyDeveloperDay added a comment. enum { A, B, C, } ShortEnum1, ShortEnum2; I've seen this before maybe with regard to something else, but can't quite recall. (maybe a bug in the bug tracker) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[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

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

[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

curdeius requested changes to this revision. curdeius added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Format/TokenAnnotator.cpp:3679-3680 +if (remainingFile[whileIndex] != '\n' && +(remainingFile[whileIndex] == ' '

[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

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

[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

HazardyKnusperkeks added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:3690-3691 +return (isAllowedByAfterEnum && isAllowedByShortEnums) || (Line.startsWith(tok::kw_class) && Style.BraceWrapping.AfterClass) ||

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

curdeius added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:3669-3688 +bool lineContainsBreakingTokens = false; +const FormatToken *breakingSearchToken = +while ((breakingSearchToken = breakingSearchToken->Next)) { + bool hasBreakingComma =

[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

HazardyKnusperkeks added a comment. In D93938#2616044 , @atirit wrote: > In D93938#2614825 , > @HazardyKnusperkeks wrote: > >> If the bugs are (very) similar, I could live with one fix for both. >> Otherwise you

[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

HazardyKnusperkeks added a comment. In D93938#2613096 , @atirit wrote: > In D93938#2612952 , > @HazardyKnusperkeks wrote: > >> In my opinion you should then, either temporarily deactivate the test, or >> fix the

[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.

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

+1 we are not going to land this with a failing or removed test On Tue, 9 Mar 2021 at 07:29, Marek Kurdej via Phabricator < revi...@reviews.llvm.org> wrote: > curdeius added a comment. > > In D93938#2612952 , > @HazardyKnusperkeks wrote: > > > In my

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

curdeius 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

HazardyKnusperkeks added a comment. In D93938#2612908 , @atirit wrote: > In D93938#2610568 , @curdeius wrote: > >> I hadn't had another look, because the CI still shows the test `AfterEnum` >> to be failing. > > I

[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

curdeius added a comment. I hadn't had another look, because the CI still shows the test `AfterEnum` to be failing. Ping me when it's green please. 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. 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

MyDeveloperDay accepted this revision. MyDeveloperDay added a comment. LGTM 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 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

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:1347 Style.AllowShortEnumsOnASingleLine = false; + verifyFormat("enum {\n" + " A,\n" + " B,\n" + " C\n" + "} ShortEnum1,

[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

MyDeveloperDay requested changes to this revision. MyDeveloperDay added a comment. This revision now requires changes to proceed. I'm sorry, but now you are missing the files from the review, I think you diffing against your own commits and not commit that are in the repo. This makes the review

[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

MyDeveloperDay added a comment. we don't commit with failing tests so lets understand why it fails. Can you add the tests without multiple names for the enum 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 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

MyDeveloperDay added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:1346 verifyFormat("enum { A, B, C } ShortEnum1, ShortEnum2;", Style); Style.AllowShortEnumsOnASingleLine = false; + verifyFormat("enum {\n"

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

MyDeveloperDay added a comment. 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 this one line change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

HazardyKnusperkeks added a comment. In D93938#2476432 , @atirit wrote: > 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

[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

curdeius added a comment. In D93938#2476186 , @MyDeveloperDay wrote: > I think we are missing some clarity in this bug as to what the actual problem > is, I do agree the test looks wrong, I agree on this. If like to see a more exhaustive test suite for

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

MyDeveloperDay added a comment. I think we are missing some clarity in this bug as to what the actual problem is, I do agree the test looks wrong, This seems to be that in "Attach" mode, then AllowShortEnumsOnASingleLine=false doesn't attach the brace. I'm somewhat struggling to understand

[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

MyDeveloperDay added a comment. Take the following example: enum { A, B, C, D, E, F, G, H, I } Short; And the following minimal .clang-format --- ColumnLimit: 10 BreakBeforeBraces: Custom BraceWrapping: AfterEnum: true To use AfterEnum you must be using "Custom" for

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

MyDeveloperDay added a comment. I think if you think you have a bug that you log it in the bug tracker and we track it with this issue. 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 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: [clang-format] Fixed AfterEnum handling

curdeius requested changes to this revision. curdeius added a comment. This revision now requires changes to proceed. Please add and fix tests. Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2485 - if (!Style.AllowShortEnumsOnASingleLine) -addUnwrappedLine();