Skip to content

[clang-format] Fix a bug in annotating function declaration names #76206

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

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

owenca
Copy link
Contributor

@owenca owenca commented Dec 22, 2023

Annotates function declaration names having unnamed parameters.

Annotates function declaration names having unnamed parameters.
@llvmbot
Copy link
Member

llvmbot commented Dec 22, 2023

@llvm/pr-subscribers-clang-format

Author: Owen Pan (owenca)

Changes

Annotates function declaration names having unnamed parameters.


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

2 Files Affected:

  • (modified) clang/lib/Format/TokenAnnotator.cpp (+2-1)
  • (modified) clang/unittests/Format/TokenAnnotatorTest.cpp (+7)
diff --git a/clang/lib/Format/TokenAnnotator.cpp b/clang/lib/Format/TokenAnnotator.cpp
index f3551af3424396..3ac3aa3c5e3a22 100644
--- a/clang/lib/Format/TokenAnnotator.cpp
+++ b/clang/lib/Format/TokenAnnotator.cpp
@@ -3403,7 +3403,8 @@ static bool isFunctionDeclarationName(bool IsCpp, const FormatToken &Current,
       continue;
     }
     if (Tok->is(tok::kw_const) || Tok->isSimpleTypeSpecifier() ||
-        Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis)) {
+        Tok->isOneOf(TT_PointerOrReference, TT_StartOfName, tok::ellipsis,
+                     TT_TypeName)) {
       return true;
     }
     if (Tok->isOneOf(tok::l_brace, TT_ObjCMethodExpr) || Tok->Tok.isLiteral())
diff --git a/clang/unittests/Format/TokenAnnotatorTest.cpp b/clang/unittests/Format/TokenAnnotatorTest.cpp
index 8e6935319b2f3d..2cafc0438ffb46 100644
--- a/clang/unittests/Format/TokenAnnotatorTest.cpp
+++ b/clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -1718,6 +1718,13 @@ TEST_F(TokenAnnotatorTest, UnderstandsFunctionDeclarationNames) {
   ASSERT_EQ(Tokens.size(), 14u) << Tokens;
   EXPECT_TOKEN(Tokens[3], tok::identifier, TT_Unknown);
   EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_FunctionTypeLParen);
+
+  auto Style = getLLVMStyle();
+  Style.TypeNames.push_back("time_t");
+  Tokens = annotate("int iso_time(time_t);", Style);
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::identifier, TT_FunctionDeclarationName);
+  EXPECT_TOKEN(Tokens[3], tok::identifier, TT_TypeName);
 }
 
 TEST_F(TokenAnnotatorTest, UnderstandsCtorAndDtorDeclNames) {

@owenca
Copy link
Contributor Author

owenca commented Dec 22, 2023

See here for background info.

@mattmundell
Copy link

mattmundell commented Dec 22, 2023

Thanks for doing this. Does it mean we would have to add every type we use to our config? It would be much more convenient if clang-format treated the argument list the same way that it treats in definitions. For example

printf "int\niso_time(time_t) { return 1; }\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

gives

int
iso_time(time_t) {
  return 1;
}

whereas

printf "int\niso_time(time_t);\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

will need a config edit to work.

Other examples that are currently failing to line break:

printf "int\niso_time(struct example);\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'
printf "typedef long long int rowid_t;\nint\niso_time(rowid_t);\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

@HazardyKnusperkeks
Copy link
Contributor

Thanks for doing this. Does it mean we would have to add every type we use to our config? It would be much more convenient if clang-format treated the argument list the same way that it treats in definitions. For example

printf "int\niso_time(time_t) { return 1; }\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

gives

int
iso_time(time_t) {
  return 1;
}

whereas

printf "int\niso_time(time_t);\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

will need a config edit to work.

clang-format doesn't know what are types, except for the keywords. So there is no way to decide if this is a function declaration without naming the parameters, or a variable declaration with a constructor call.

@mattmundell
Copy link

clang-format doesn't know what are types, except for the keywords. So there is no way to decide if this is a function declaration without naming the parameters, or a variable declaration with a constructor call.

This is C so I guess it can only be a function declaration. Anyway, it's minor to work around, thanks again.

@owenca owenca merged commit f8f8926 into llvm:main Dec 23, 2023
@owenca owenca deleted the function-decl-name branch December 23, 2023 06:51
@HazardyKnusperkeks
Copy link
Contributor

clang-format doesn't know what are types, except for the keywords. So there is no way to decide if this is a function declaration without naming the parameters, or a variable declaration with a constructor call.

This is C so I guess it can only be a function declaration. Anyway, it's minor to work around, thanks again.

This boils down to, that for clang-format C doesn't exist. It's all parsed as C++.

@owenca
Copy link
Contributor Author

owenca commented Mar 10, 2024

Thanks for doing this. Does it mean we would have to add every type we use to our config? It would be much more convenient if clang-format treated the argument list the same way that it treats in definitions. For example

printf "int\niso_time(time_t) { return 1; }\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

gives

int
iso_time(time_t) {
  return 1;
}

whereas

printf "int\niso_time(time_t);\n" | clang-format -style='{ AlwaysBreakAfterReturnType: All }'

will need a config edit to work.

@mattmundell you don't need to add time_t and other common C/C++ types to TypeNames after 0baef3b.

@mattmundell
Copy link

Thanks @owenca, I added an argument name instead (cb983df).

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.

4 participants