Revert "[clang-format] Annotate ::operator and Foo::operator correctly" #164670
Conversation
|
@llvm/pr-subscribers-clang-format Author: None (PiJoules) ChangesReverts llvm/llvm-project#164048 This led to a regression in clang-format where a space gets added in between the parameter type and becomes Full diff: https://github.com/llvm/llvm-project/pull/164670.diff 2 Files Affected:
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) {
|
|
|
frobtech
left a comment
frobtech
left a comment
There was a problem hiding this comment.
This is a clear regression. I hope we can get a regression test case added to catch it.
frobtech
left a comment
frobtech
left a comment
There was a problem hiding this comment.
This is a clear regression. I hope we can get a regression test case added to catch it.
|
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 |
No, it's not. See #164048 (comment).
The relevant parts of the policy that show why you shouldn't have reverted:
|
…ly" (#164670) This reverts commit 99abda7. See #164670 (comment)
…tor correctly" (#164670) This reverts commit 99abda7. See llvm/llvm-project#164670 (comment)
…ly" (llvm#164670) This reverts commit 99abda7. See llvm#164670 (comment)
#111115 (which "fixed" #164866 by chance) was effectively reverted by #164048 because of the regression #160513. Again, please see #164048 (comment). |
|
The recent activity here is #164048, then #164670, then a948f25? Reverting a commit from the 18th on the 22nd is within the policy, although I'd usually prefer to give the author of the patch a little more time to respond before merging. Every commit has a goal to fix something, but the point of the revert policy is to ensure stability for people closely tracking top of tree. Maintaining that stability is higher priority than a general bugfix, even if it's fixing an important bug. If you're causing immediate breakage, the right response is to back out, then reland when you have a handle on the regressions. The fact that the bugfix is fixing a regression from a year ago isn't really relevant; keeping stability is the highest priority. Not sure what we want to do for the branch; we can maybe take a bit more time there to figure out the right approach, once we know what the fix for top-of-tree looks like. |
|
As per @efriedma-quic's comment I'll go ahead and revert the commit again to prevent the immediate breakage. @owenca by no means do we want to keep your fix reverted, but we would like to keep ToT stable. If needed, we can help out with a reland that both fixes the old regression and prevents this new bug. |
… correctly" (llvm#164670)" This reverts commit 50ca1f4. Reverting because this leads to the bug on ToT described in llvm#164866. The original fix addresses an old regression which we'd still like to land eventually. See the discussion in llvm#164670 for more context.
…o::operator… (#165038) … correctly" (#164670)" This reverts commit 50ca1f4. Reverting because this leads to the bug on ToT described in llvm/llvm-project#164866. The original fix addresses an old regression which we'd still like to land eventually. See the discussion in llvm/llvm-project#164670 for more context.
@PiJoules, I don't know if @efriedma-quic's comment was an endorsement of your original hasty (and incomplete) revert, but it didn't address the first part of the policy I quoted above. Please stop forcing your incomplete revert in without first consulting the clang-format team @mydeveloperday, @HazardyKnusperkeks, etc. If we reach the consensus to fix #160513 by reverting instead of by fixing forward, you need to identify the proper "set of patches" to revert and open a pull request for approval by the clang-format team. That set must include both #111115 and #164048 and most likely one or more commits in between. cc @JohnC32 |
…r… (#165038) This reverts commit bd27abc. See #164670 (comment)
…oo::operator… (#165038) This reverts commit bd27abc. See llvm/llvm-project#164670 (comment)
As far as I can tell, what happened is @owenca committed a patch on Oct 17, which broke a workflow for @PiJoules, which was reported on Oct 21. He asked for a immediate fix/revert, which did not happen, then he reverted on Oct 22. After that point, this turned into a revert war. Currently, the Oct 17 patch is in. There's also some discussion of a patch which landed last year, but from my perspective it's not relevant. I apologize for not being 100% explicit about what I expected to happen in my previous message. In the interest of resolving the immediate breakage quickly without further devolving into revert wars, I'm going to be extremely explicit here: @owenca You have 48 hours from now to resolve the issue @PiJoules reported against main. Either revert to the state before Oct 17, or fix the issue. If you don't solve it within 48 hours from this message (deadline is 10pm pdt Monday Oct 27), you're not allowed to touch any code related to this issue without getting approval from @PiJoules or me. Please acknowledge this as soon as you see it. @PiJoules Don't do anything for 48 hours. After the deadline, you can revert to the state before Oct 17. After this is resolved, let's have a Discourse discussion to figure out how we can avoid future revert wars. |
…y" (llvm#164670) Reverts llvm#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 { ```
…ly" (llvm#164670) This reverts commit 99abda7. See llvm#164670 (comment)
llvm#165038) … correctly" (llvm#164670)" This reverts commit 50ca1f4. Reverting because this leads to the bug on ToT described in llvm#164866. The original fix addresses an old regression which we'd still like to land eventually. See the discussion in llvm#164670 for more context.
…r… (llvm#165038) This reverts commit bd27abc. See llvm#164670 (comment)
Here is a recap of a more detailed timeline before @PiJoules escalated the issue to @llvm/clang-area-team:
|
…y" (llvm#164670) Reverts llvm#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 { ```
…ly" (llvm#164670) This reverts commit 99abda7. See llvm#164670 (comment)
llvm#165038) … correctly" (llvm#164670)" This reverts commit 50ca1f4. Reverting because this leads to the bug on ToT described in llvm#164866. The original fix addresses an old regression which we'd still like to land eventually. See the discussion in llvm#164670 for more context.
…r… (llvm#165038) This reverts commit bd27abc. See llvm#164670 (comment)
…y" (llvm#164670) Reverts llvm#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 { ```
…ly" (llvm#164670) This reverts commit 99abda7. See llvm#164670 (comment)
llvm#165038) … correctly" (llvm#164670)" This reverts commit 50ca1f4. Reverting because this leads to the bug on ToT described in llvm#164866. The original fix addresses an old regression which we'd still like to land eventually. See the discussion in llvm#164670 for more context.
…r… (llvm#165038) This reverts commit bd27abc. See llvm#164670 (comment)
Reverts #164048
This led to a regression in clang-format where a space gets added in between the parameter type and
&. For example, thisbecomes