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

[Mike Weller via Phabricator via cfe-commits]

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 have already been commenting on

Apologies, late to the party here... this fix suggested by curdeius does indeed 
fix the issue:

> 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 negation). It will count characters other than 
> a newline and it will only count a series of consecutive spaces as one char. 
> WDYT?




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

atirit wrote:
> curdeius wrote:
> > atirit wrote:
> > > curdeius wrote:
> > > > I don't really understand these conditions on spaces. Could you explain 
> > > > your intent, please?
> > > > You really need to add specific tests for that, playing with the value 
> > > > of ColumnLimit, adding spacing etc.
> > > Repeated spaces, e.g. `enum {   A,  B, C } SomeEnum;` are removed during 
> > > formatting. Since they wouldn't be part of the formatted line, they 
> > > shouldn't be counted towards the column limit. Only one space need be 
> > > considered. Removed spaces, e.g. `enum{A,B,C}SomeEnum;` are handled by 
> > > the fact that `clang-format` runs multiple passes. On the first pass, 
> > > spaces would be added. On the second pass, assuming the line is then too 
> > > long, the above code would catch it and break up the enum.
> > > 
> > > I'll add unit tests to check if spaces are being handled correctly.
> > 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 negation). It will count characters other 
> > than a newline and it will only count a series of consecutive spaces as one 
> > char. WDYT?
> Ah yes, that's my bad. Must have made a typo. Fixed in the next commit.
> 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 negation). It will count characters other than 
> a newline and it will only count a series of consecutive spaces as one char. 
> WDYT?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mike Weller via Phabricator via cfe-commits]

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) {
FormatStyle Style = getLLVMStyle();
Style.AllowShortEnumsOnASingleLine = true;
Style.ColumnLimit = 40;
  
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.AfterEnum = false;
  
verifyFormat("enum {\n"
 "  ZERO = 0,\n"
 "  ONE = 1,\n"
 "  TWO = 2,\n"
 "  THREE = 3\n"
 "} LongEnum;",
 Style);
Style.BreakBeforeBraces = FormatStyle::BS_Custom;
Style.BraceWrapping.AfterEnum = true;
verifyFormat("enum\n"
 "{\n"
 "  ZERO = 0,\n"
 "  ONE = 1,\n"
 "  TWO = 2,\n"
 "  THREE = 3\n"
 "} LongEnum;",
 Style);
  }

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 have already been commenting on


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Mike Weller via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44609: [clang-format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

[Mike Weller via Phabricator via cfe-commits]

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/D44609/new/

https://reviews.llvm.org/D44609

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits