[PATCH] D103678: [Format] Fix incorrect pointer/reference detection
Marek Kurdej via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 8 05:09:02 PDT 2021
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
More information about the cfe-commits
mailing list