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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
