[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Marek Kurdej via Phabricator via cfe-commits]

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