Skip to content

[clang-format] Handle templated elaborated type specifier in function… #77013

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 8 commits into from
Jan 20, 2024

Conversation

XDeme1
Copy link
Contributor

@XDeme1 XDeme1 commented Jan 4, 2024

… return type.

The behavior now is consistent with the non template version.
Enabled and updated old test.

… return type.

The behaviour now is consistent with the non templated version
@llvmbot
Copy link
Member

llvmbot commented Jan 4, 2024

@llvm/pr-subscribers-clang-format

Author: None (XDeme)

Changes

… return type.

The behavior now is consistent with the non template version.
Enabled and updated old test.


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

2 Files Affected:

  • (modified) clang/lib/Format/UnwrappedLineParser.cpp (+8)
  • (modified) clang/unittests/Format/FormatTest.cpp (+1-3)
diff --git a/clang/lib/Format/UnwrappedLineParser.cpp b/clang/lib/Format/UnwrappedLineParser.cpp
index 684609747a5513..aaff6319dd45ef 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -3914,7 +3914,15 @@ void UnwrappedLineParser::parseRecord(bool ParseAsExpr) {
   // (this would still leave us with an ambiguity between template function
   // and class declarations).
   if (FormatTok->isOneOf(tok::colon, tok::less)) {
+    int AngleNestingLevel = 0;
     do {
+      if (FormatTok->is(tok::less))
+        ++AngleNestingLevel;
+      else if (FormatTok->is(tok::greater))
+        --AngleNestingLevel;
+
+      if (AngleNestingLevel == 0 && FormatTok->is(tok::r_paren))
+        break;
       if (FormatTok->is(tok::l_brace)) {
         calculateBraceTypes(/*ExpectClassBody=*/true);
         if (!tryToParseBracedList())
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 762fc8254bdfc9..f304407d0ce2f4 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -14583,9 +14583,7 @@ TEST_F(FormatTest, UnderstandContextOfRecordTypeKeywords) {
   verifyFormat("template <> struct X < 15, i<3 && 42 < 50 && 33 < 28> {};");
   verifyFormat("int i = SomeFunction(a<b, a> b);");
 
-  // FIXME:
-  // This now gets parsed incorrectly as class definition.
-  // verifyFormat("class A<int> f() {\n}\nint n;");
+  verifyFormat("class A<int> f() {}\nint n;");
 
   // Elaborate types where incorrectly parsing the structural element would
   // break the indent.

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.

Ok, one last thing. From what I figured it was wrongly detected as class definition. So you could add a token annotator test, which verifies that the { is not an ClassLBrace?

@XDeme1
Copy link
Contributor Author

XDeme1 commented Jan 10, 2024

Edit: I have updated it to check for a Identifier that is not a macro before a tok::l_paren, this will fix the issue I mentioned below, if this is not recommended I could just revert this change and leave a updated comment about this case.

I have updated the comment a little, because there is some case we can't handle:

template<typename T>
struct Goo<T> F() {}

we can't distinguish if this is a class or function. We could check if F() is macro by checking
if it is all uppercase, to identify a macro, but I am not sure if this is good.

If this is OK, I could do these checks, and possibly remove the entire comment.

@owenca owenca merged commit a464e05 into llvm:main Jan 20, 2024
@XDeme1 XDeme1 deleted the ElaboratedClassSpecifierReturn branch January 20, 2024 22:51
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.

5 participants