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

[Marek Kurdej via Phabricator via cfe-commits]

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 wrote:
> curdeius wrote:
> > strlen? Please use `StringRef::size()`.
> For `FormatToken.TokenText`, `StringRef::size()` is set to the length of the token, not of the stored text. Please see `clang/lib/Format/FormatTokenLexer.cpp:1047`. Changing that line breaks nearly every format test so I'm assuming that it is correct. A `strlen` or equivalent is necessary here.
Then it should be something like the line's length, no? Using `strlen` will be very expensive on non-snippets, as it `strlen` will traverse the string until its end (so possibly until the end of file) for each invocation of `mustBreakBefore` (if it goes into this condition of course).

I only see one failing check in the test `FormatTest.FormatsTypedefEnum` when using `TokenText.size()`:
```
  verifyFormat("typedef enum\n"
               "{\n"
               "  ZERO = 0,\n"
               "  ONE = 1,\n"
               "  TWO = 2,\n"
               "  THREE = 3\n"
               "} LongEnum;",
               Style);
```

You might need to add more tests in `AfterEnum` to test the behaviour of this part if the line is just below/above the limit.

Also, that's just a hack, but I made all tests pass with:
```
    assert(Line.Last);
    assert(Line.Last->TokenText.data() >= Right.TokenText.data());
    auto isAllowedByShortEnums = [&]() {
      if (!Style.AllowShortEnumsOnASingleLine ||
          (Line.Last->TokenText.data() - Right.TokenText.data() +
           Right.TokenText.size() + Right.OriginalColumn) >
              Style.ColumnLimit)
```
I haven't given it too much thought though and am unsure whether there are cases where the above assertions will fail.


================
Comment at: clang/unittests/Format/FormatTest.cpp:13426-13435
   verifyFormat("enum X\n"
                "{\n"
                "  Y = 0,\n"
                "}\n",
                AllmanBraceStyle);
   verifyFormat("enum X\n"
                "{\n"
----------------
I would then put `AllmanBraceStyle.AllowShortEnumsOnASingleLine = ...;` just before these enum tests and put the old value back afterwards.
Also, as @HazardyKnusperkeks suggested test both false and true.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938