curdeius accepted this revision.
curdeius added a comment.
LGTM modulo nits.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:232
if (Left->is(TT_OverloadedOperatorLParen)) {
- Contexts.back().IsExpression = false;
+ // Find the previous kw_operator token
+ FormatToken *Prev = Left;
----------------
Nit: full-stop at the end.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:236
+ Prev = Prev->Previous;
+ assert(Prev);
+ }
----------------
MyDeveloperDay wrote:
> Do we need to worry about `Prev` ever being null? Does the assert ever fire?
> if not why have it in the first place?
>
> We'll crash if it is, do we want to guard against that?
Disclaimer: I'm in the pro-assert camp :).
In general, I'd preferably keep this assert because people sometimes use
Release+Assertions build. Even more those that report bugs.
And it's much nicer and easier to fix an assertion failure that is well
defined and precisely localised than a less clearer report of a crash.
Of course, in a build without assertions, it will crash if `Prev` is null, but
still it will be easier to find the bug when trying to reproduce.
For this specific case, this assertion should never fire because otherwise it
would mean that we detected had `Left` being `TT_OverloadedOperatorLParen` but
there was no corresponding `tok::kw_operator` (or just `operator`). So it would
mean that there's a bug in (probably) token annotator. So yeah, it falls into
"should never happen" category about which assertions are all about IMO.
================
Comment at: clang/unittests/Format/FormatTest.cpp:8283-8284
+ verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+ // TODO:
+ // Calling an operator as a non-member function is hard to distinguish
+ // verifyFormat("void f() { operator*(a & a); }");
----------------
curdeius wrote:
> Nuullll wrote:
> > This patch doesn't fix these, i.e. when operators are called as non-member
> > functions.
> > The call sites seem to be marked as function declarations.
> Is there a bug report on that? If so, please add a link to the comment. If
> no, could you please create one?
> Or is it really something that will never be possible to distinguish (hmm,
> this would be strange)?
Nit: full-stop.
================
Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+ // TODO:
+ // Calling an operator as a non-member function is hard to distinguish
+ // verifyFormat("void f() { operator*(a & a); }");
+ // verifyFormat("void f() { operator&(a, b * b); }");
----------------
Nuullll wrote:
> This patch doesn't fix these, i.e. when operators are called as non-member
> functions.
> The call sites seem to be marked as function declarations.
Is there a bug report on that? If so, please add a link to the comment. If no,
could you please create one?
Or is it really something that will never be possible to distinguish (hmm, this
would be strange)?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D103678/new/
https://reviews.llvm.org/D103678
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
