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

[Ally Tiritoglu via Phabricator via cfe-commits]

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 wrote:
> 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.
If I use `TokenText.size()`, the line length check will always claim that the line is short enough. I'll be adding a unit test for this. However, you're right that a `strlen` is a bad idea here. I hadn't realised it would consume the entire file. I'll try to figure out a more efficient method of checking the line length.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938