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

[Mike Weller via Phabricator via cfe-commits]

Userbla added a comment.

In D93938#2833045 <https://reviews.llvm.org/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