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] Fix a bug in annotating CastRParen #102261

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Conversation

owenca
Copy link
Contributor

@owenca owenca commented Aug 7, 2024

Fixes #102102.

@llvmbot
Copy link
Member

llvmbot commented Aug 7, 2024

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Fixes #102102.


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

2 Files Affected:

  • (modified) clang/lib/Format/TokenAnnotator.cpp (+10-1)
  • (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+7)
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index 4ed3e9d0e8e855..e1c9f37c6e8dc2 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -2874,9 +2874,18 @@ class AnnotatingParser {
       return false;
 
     // Search for unexpected tokens.
-    for (auto *Prev = BeforeRParen; Prev != LParen; Prev = Prev->Previous)
+    for (auto *Prev = BeforeRParen; Prev != LParen; Prev = Prev->Previous) {
+      if (Prev->is(tok::r_paren)) {
+        Prev = Prev->MatchingParen;
+        if (!Prev)
+          return false;
+        if (Prev->is(TT_FunctionTypeLParen))
+          break;
+        continue;
+      }
       if (!Prev->isOneOf(tok::kw_const, tok::identifier, tok::coloncolon))
         return false;
+    }
 
     return true;
   }
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 6b71d0d18cf7a4..cbbe550a79ebc2 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -740,6 +740,13 @@ TEST_F(TokenAnnotatorTest, UnderstandsCasts) {
   EXPECT_TOKEN(Tokens[10], tok::r_paren, TT_Unknown);
   EXPECT_TOKEN(Tokens[11], tok::amp, TT_BinaryOperator);
 
+  Tokens = annotate("func((void (*)())&a);");
+  ASSERT_EQ(Tokens.size(), 15u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_FunctionTypeLParen);
+  EXPECT_TOKEN(Tokens[5], tok::star, TT_PointerOrReference);
+  EXPECT_TOKEN(Tokens[9], tok::r_paren, TT_CastRParen);
+  EXPECT_TOKEN(Tokens[10], tok::amp, TT_UnaryOperator);
+
   auto Style = getLLVMStyle();
   Style.TypeNames.push_back("Foo");
   Tokens = annotate("#define FOO(bar) foo((Foo)&bar)", Style);

@prj-
Copy link

prj- commented Aug 7, 2024

I'm sorry I'm not familiar with LLVM release cycle, but is it too late for this to make it into the release/19.x branch?

@owenca owenca added this to the LLVM 19.X Release milestone Aug 8, 2024
@owenca
Copy link
Contributor Author

owenca commented Aug 8, 2024

I'm sorry I'm not familiar with LLVM release cycle, but is it too late for this to make it into the release/19.x branch?

It's not too late, but in general we backport bug fixes only if the bugs are labeled as invalid-code-generation, crash-on-valid, or regression. In this particular case though, the fix is localized and fairly simple, and has been approved by all clang-format maintainers, so I've decided to backport it to 19.x per your request.

@owenca owenca merged commit 8c7a038 into llvm:main Aug 8, 2024
9 checks passed
@owenca owenca deleted the 102102 branch August 8, 2024 04:05
@owenca
Copy link
Contributor Author

owenca commented Aug 8, 2024

/cherry-pick 8c7a038

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2024

/pull-request #102419

@prj-
Copy link

prj- commented Aug 8, 2024

Thank you very much.

@prj-
Copy link

prj- commented Aug 10, 2024

This introduced a regression though.

-      for (i = 0, k = 1; i < ((PetscInt)local_n0) * partial_dim; i++, k++) {
+      for (i = 0, k = 1; i < ((PetscInt)local_n0)*partial_dim; i++, k++) {

@prj-
Copy link

prj- commented Aug 10, 2024

Nevermind, this bug (not sure if it's a bug or not) is already present in clang-format 18. See #102727.

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 10, 2024
kstoimenov pushed a commit to kstoimenov/llvm-project that referenced this pull request Aug 15, 2024
@krasimirgg
Copy link
Contributor

It appears that this introduced a regression in code like this:

int d = 0;
int c(int i) { return i; }
void a() { double b = ((double)c(0)) * d / (double)0; }

After this patch, the last line gets formatted as:

void a() { double b = ((double)c(0))*d / (double)0; }

@prj-
Copy link

prj- commented Aug 28, 2024

What's your version?

$ clang-format-20 gh.txt
void a() { double b = ((double)c(0)) * d / (double)0; }

@krasimirgg
Copy link
Contributor

Ohh sorry, I had started with a stale clang-format commit. This is a non-issue at HEAD.

@kadircet
Copy link
Member

kadircet commented Sep 6, 2024

there still seems to be a regression when the operator is -, filed #107568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[clang-format] Spacing with function pointer
8 participants