Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format] Improve function pointer CastRParen detection. #126019

Closed
wants to merge 3 commits into from

Conversation

rmarker
Copy link
Contributor

@rmarker rmarker commented Feb 6, 2025

Fixes #125012

@llvmbot
Copy link
Member

llvmbot commented Feb 6, 2025

@llvm/pr-subscribers-clang-format

Author: None (rmarker)

Changes

Fixes #125012


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

2 Files Affected:

  • (modified) clang/lib/Format/TokenAnnotator.cpp (+15-2)
  • (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+5)
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index f25332e3a5f4e1a..fc84f29dd04389b 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2915,14 +2915,27 @@ class AnnotatingParser {
     if (Prev->is(tok::r_paren)) {
       if (Prev->is(TT_CastRParen))
         return false;
+
+      // Check for the second pair of parentheses.
       Prev = Prev->MatchingParen;
       if (!Prev)
         return false;
+
+      // Now check for the first pair of parentheses.
       Prev = Prev->Previous;
       if (!Prev || Prev->isNot(tok::r_paren))
         return false;
-      Prev = Prev->MatchingParen;
-      return Prev && Prev->is(TT_FunctionTypeLParen);
+      const auto* PrevMatchingParen = Prev->MatchingParen;
+      if (!PrevMatchingParen)
+        return false;
+
+      // We can quickly tell it is a function pointer type if the paren is the
+      // right type.
+      if (PrevMatchingParen->isNot(TT_Unknown))
+        return PrevMatchingParen->is(TT_FunctionTypeLParen);
+
+      // Otherwise check for the trailing * in the parentheses.
+      return Prev->Previous && Prev->Previous->is(tok::star);
     }
 
     // Search for unexpected tokens.
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 1b09c4570345622..5dcb9b94a5bb2f5 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -874,6 +874,11 @@ TEST_F(TokenAnnotatorTest, UnderstandsCasts) {
   EXPECT_TOKEN(Tokens[14], tok::r_paren, TT_CastRParen);
   EXPECT_TOKEN(Tokens[15], tok::amp, TT_UnaryOperator);
 
+  Tokens = annotate("func((foo(bar::*)(void))&a);");
+  ASSERT_EQ(Tokens.size(), 18u) << Tokens;
+  EXPECT_TOKEN(Tokens[12], tok::r_paren, TT_CastRParen);
+  EXPECT_TOKEN(Tokens[13], tok::amp, TT_UnaryOperator);
+
   auto Style = getLLVMStyle();
   Style.TypeNames.push_back("Foo");
   Tokens = annotate("#define FOO(bar) foo((Foo)&bar)", Style);

Copy link

github-actions bot commented Feb 6, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@HazardyKnusperkeks HazardyKnusperkeks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please wait for @owenca or @mydeveloperday.

@@ -874,6 +874,11 @@ TEST_F(TokenAnnotatorTest, UnderstandsCasts) {
EXPECT_TOKEN(Tokens[14], tok::r_paren, TT_CastRParen);
EXPECT_TOKEN(Tokens[15], tok::amp, TT_UnaryOperator);

Tokens = annotate("func((foo(bar::*)(void))&a);");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's foo here? Can you show an example in Compiler Explorer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here foo is a type.

It looks like I accidentally trimmed too much when making the test case. Whoops.
Good catch.
It should be func((foo(bar::*)(void))&bar::a);
I've pushed a commit to adjust that.

Here is a Compiler Explorer link with what I was going for.
https://godbolt.org/z/Yhn65zx8P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So foo is a return type. Then the ( that follows should be annotated as a FunctionLParen, and there should be a space between them like in int (*)().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was so focused on the behaviour from 19, that I didn't realise about the space after the return type.
Yeah, it seems like even with missing the annotation of the left paren clang-format 19, managed to get lucky with the right cast paren. With the extra fix in 20 revealing the underlying issue.
Nice work.

@owenca
Copy link
Contributor

owenca commented Feb 8, 2025

The root cause is that the l_paren after the return type is not annotated. That's why the space between them is missing.

@owenca
Copy link
Contributor

owenca commented Feb 8, 2025

See #126340.

@rmarker rmarker closed this Feb 9, 2025
@rmarker rmarker deleted the 125012 branch February 9, 2025 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-format] Misformatted address-of operator after cast on 20 versus 19
4 participants