[Format] Fix incorrect pointer/reference detection

[Nuullll]

Summary

https://llvm.org/PR50568

When an overloaded operator is called, its argument must be an
expression.

Before:

void f() { a.operator()(a *a); }

After:

void f() { a.operator()(a * a); }

Signed-off-by: Yilong Guo <[e-mail]>

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

243

feels like we are not testing this situation, please add those tests

clang/unittests/Format/FormatTest.cpp

8783

can you add

void f() { a.operator()(*a * *a); }

[Nuullll]

Add more tests.

[Nuullll]

clang/lib/Format/TokenAnnotator.cpp

243

I added some in FormatTest.UnderstandsOverloadedOperators

clang/unittests/Format/FormatTest.cpp

8283–8286

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.

[Nuullll]

Pre-check failure: the github server is currently down.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

236

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?

[curdeius]

LGTM modulo nits.

clang/lib/Format/TokenAnnotator.cpp

232–244

Nit: full-stop at the end.

236

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.

clang/unittests/Format/FormatTest.cpp

8283–8284

Nit: full-stop.

8283–8286

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)?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

236

To be honest I'm in the assert and guard as well camp (if such a camp exists ;-)

I have this discussion regularly with the other developers in my team, the use assert is useless other than for our assertion builds (which in high performance code, almost no one runs, until there is a problem!)

The assert triggers in my mind that it MIGHT be null (but generally shouldn't) so in which case we have to guard as well for the rare case, but I like the assert.. but please of the form:

assert(condition && "Some nice string telling me exactly what went wrong")

When the assert fires I get a nice error message, not just assert failed with 0

https://llvm.org/docs/CodingStandards.html#assert-liberally

[MyDeveloperDay]

clang/unittests/Format/FormatTest.cpp

8283–8286

Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)

We do appreciate your patch!!

[Nuullll]

Minor updates.

[Nuullll]

clang/lib/Format/TokenAnnotator.cpp

236

@curdeius @MyDeveloperDay Thanks for the explanations. I've added an error message.

clang/unittests/Format/FormatTest.cpp

8283–8286

@curdeius

Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?

No. I've created one and added the link in the comment!

Or is it really something that will never be possible to distinguish (hmm, this would be strange)?

I think it should be distinguishable, at least there should be no return type before the operator in the context of non-member function invocation. I'd like to fix this in a separate patch, later. I need some time to figure it out :)

8283–8286

Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)

We do appreciate your patch!!

@MyDeveloperDay Thanks for reviewing this. I will try to fix these in another patch. It's always exciting to contribute to llvm!

[HazardyKnusperkeks]

Do you need some one to commit this? If yes please state name and email, some one will chime in to commit it.

[Nuullll]

In D103678#2811302, @HazardyKnusperkeks wrote:

Do you need some one to commit this? If yes please state name and email, some one will chime in to commit it.

Yes, please someone help commit this.

Name: Yilong Guo
Email: [e-mail]

Thanks in advance!

clang/lib/Format/TokenAnnotator.cpp

if (FormatToken *MaybeSel = Left->Previous) {
       // @selector( starts a selector.
       if (MaybeSel->isObjCAtKeyword(tok::objc_selector) && MaybeSel->Previous &&
           MaybeSel->Previous->is(tok::at)) {
         StartsObjCMethodExpr = true;
       }
     }
 
     if (Left->is(TT_OverloadedOperatorLParen)) {
-      Contexts.back().IsExpression = false;
+      // Find the previous kw_operator token.
+      FormatToken *Prev = Left;
+      while (!Prev->is(tok::kw_operator)) {
+        Prev = Prev->Previous;
+        assert(Prev && "Expect a kw_operator prior to the OperatorLParen!");
+      }
+
+      // If faced with "a.operator*(argument)" or "a->operator*(argument)",
+      // i.e. the operator is called as a member function,
+      // then the argument must be an expression.
+      bool OperatorCalledAsMemberFunction =
+          Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+      Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
     } else if (Style.Language == FormatStyle::LK_JavaScript &&
                (Line.startsWith(Keywords.kw_type, tok::identifier) ||
                 Line.startsWith(tok::kw_export, Keywords.kw_type,
                                 tok::identifier))) {
       // type X = (...);
       // export type X = (...);
       Contexts.back().IsExpression = false;
     } else if (Left->Previous &&

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsOverloadedOperators) {
 
   verifyGoogleFormat("operator void*();");
   verifyGoogleFormat("operator SomeType<SomeType<int>>();");
   verifyGoogleFormat("operator ::A();");
 
   verifyFormat("using A::operator+;");
   verifyFormat("inline A operator^(const A &lhs, const A &rhs) {}\n"
                "int i;");
+
+  // Calling an operator as a member function.
+  verifyFormat("void f() { a.operator*(); }");
+  verifyFormat("void f() { a.operator*(b & b); }");
+  verifyFormat("void f() { a->operator&(a * b); }");
+  verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+  // TODO: Calling an operator as a non-member function is hard to distinguish.
+  // https://llvm.org/PR50629
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
   verifyFormat("Deleted &operator=(const Deleted &) & = default;");
   verifyFormat("Deleted &operator=(const Deleted &) && = delete;");
   verifyFormat("SomeType MemberFunction(const Deleted &) & = delete;");
   verifyFormat("SomeType MemberFunction(const Deleted &) && = delete;");
   verifyFormat("Deleted &operator=(const Deleted &) &;");
TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
 
   // Member function reference qualifiers aren't binary operators.
   verifyFormat("string // break\n"
                "operator()() & {}");
   verifyFormat("string // break\n"
                "operator()() && {}");
   verifyGoogleFormat("template <typename T>\n"
                      "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function).
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
+  verifyFormat("void f() { a.operator()(*a & *a); }");
+  verifyFormat("void f() { a->operator()(*a * *a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
   verifyFormat("SomeType s __attribute__((unused)) (InitValue);");
   verifyFormat("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa __attribute__((unused))\n"
                "aaaaaaaaaaaaaaaaaaaaaaa(int i);");
   FormatStyle AfterType = getLLVMStyle();
   AfterType.AlwaysBreakAfterReturnType = FormatStyle::RTBS_All;