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