Revert "[clang-format] Annotate ::operator and Foo::operator correctly"

[PiJoules]

Reverts #164048

This led to a regression in clang-format where a space gets added in between the parameter type and &. For example, this

::test_anonymous::FunctionApplication& ::test_anonymous::FunctionApplication::operator=(const ::test_anonymous::FunctionApplication& other) noexcept {

becomes

::test_anonymous::FunctionApplication& ::test_anonymous::FunctionApplication::operator=(const ::test_anonymous::FunctionApplication & other) noexcept {

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: None (PiJoules)

Changes

Reverts llvm/llvm-project#164048

This led to a regression in clang-format where a space gets added in between the parameter type and &. For example, this

::test_anonymous::FunctionApplication& ::test_anonymous::FunctionApplication::operator=(const ::test_anonymous::FunctionApplication& other) noexcept {

becomes

::test_anonymous::FunctionApplication& ::test_anonymous::FunctionApplication::operator=(const ::test_anonymous::FunctionApplication & other) noexcept {

---

Full diff: https://github.com/llvm/llvm-project/pull/164670.diff

2 Files Affected:

• (modified) clang/lib/Format/TokenAnnotator.cpp (+7-3)
• (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (-5)

diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index c97a9e81eb59e..25971d2497f97 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -3791,12 +3791,18 @@ static bool isFunctionDeclarationName(const LangOptions &LangOpts,
   if (Current.is(TT_FunctionDeclarationName))
     return true;
 
-  if (Current.isNoneOf(tok::identifier, tok::kw_operator))
+  if (!Current.Tok.getIdentifierInfo())
     return false;
 
   const auto *Prev = Current.getPreviousNonComment();
   assert(Prev);
 
+  if (Prev->is(tok::coloncolon))
+    Prev = Prev->Previous;
+
+  if (!Prev)
+    return false;
+
   const auto &Previous = *Prev;
 
   if (const auto *PrevPrev = Previous.getPreviousNonComment();
@@ -3845,8 +3851,6 @@ static bool isFunctionDeclarationName(const LangOptions &LangOpts,
 
   // Find parentheses of parameter list.
   if (Current.is(tok::kw_operator)) {
-    if (Line.startsWith(tok::kw_friend))
-      return true;
     if (Previous.Tok.getIdentifierInfo() &&
         Previous.isNoneOf(tok::kw_return, tok::kw_co_return)) {
       return true;
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index ca99940890984..f3637383a0a65 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1129,11 +1129,6 @@ TEST_F(TokenAnnotatorTest, UnderstandsOverloadedOperators) {
   ASSERT_EQ(Tokens.size(), 7u) << Tokens;
   // Not TT_FunctionDeclarationName.
   EXPECT_TOKEN(Tokens[3], tok::kw_operator, TT_Unknown);
-
-  Tokens = annotate("SomeAPI::operator()();");
-  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
-  // Not TT_FunctionDeclarationName.
-  EXPECT_TOKEN(Tokens[2], tok::kw_operator, TT_Unknown);
 }
 
 TEST_F(TokenAnnotatorTest, OverloadedOperatorInTemplate) {

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

[frobtech]

This is a clear regression. I hope we can get a regression test case added to catch it.

[frobtech]

This is a clear regression. I hope we can get a regression test case added to catch it.

[HazardyKnusperkeks]

It was that urgent, that you couldn't wait for someone to handle it correctly?

[Prabhuk]

It was that urgent, that you couldn't wait for someone to handle it correctly?

Please check the developer policy on patch reversion if you have not done so already: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

[owenca]

This is a clear regression. I hope we can get a regression test case added to catch it.

No, it's not. See #164048 (comment).

It was that urgent, that you couldn't wait for someone to handle it correctly?

Please check the developer policy on patch reversion if you have not done so already: https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

The relevant parts of the policy that show why you shouldn't have reverted:

• You should be sure that reverting the change improves the stability of tip of tree. Sometimes, reverting one change in a series can worsen things instead of improving them. We expect reasonable judgment to ensure that the proper patch or set of patches is being reverted.
• Reverts should be reasonably timely. A change submitted two hours ago can be reverted without prior discussion. A change submitted two years ago should not be. Where exactly the transition point is is hard to say, but it’s probably in the handful of days in tree territory. If you are unsure, we encourage you to reply to the commit thread, give the author a bit to respond, and then proceed with the revert if the author doesn’t seem to be actively responding.

cc @mydeveloperday

[owenca]

@Prabhuk I've reverted your revert. Please open a GitHub issue for your use case, which also failed before #111115 was merged.

[PiJoules]

I filed #164866 which includes a regression test.

@Prabhuk I've reverted your revert. Please open a GitHub issue for your use case, which also failed before #111115 was merged.

I think that's the wrong patch? We reverted #164048.

[owenca]

I filed #164866 which includes a regression test.

@Prabhuk I've reverted your revert. Please open a GitHub issue for your use case, which also failed before #111115 was merged.

I think that's the wrong patch? We reverted #164048.

#111115 (which "fixed" #164866 by chance) was effectively reverted by #164048 because of the regression #160513. Again, please see #164048 (comment).