Skip to content

[Sema] Default arguments for template parameters affect ContainsUnexpandedPacks #99880

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 5 commits into from
Jul 23, 2024

Conversation

ilya-biryukov
Copy link
Contributor

@ilya-biryukov ilya-biryukov commented Jul 22, 2024

This addresses the FIXME in the code. There will be tests for the new behavior in an upcoming #86265, which also addresses other bugs that prevent exposing the wrong results of ContainsUnexpandedPacks in the outputs of the compiler without crashes.

@ilya-biryukov ilya-biryukov self-assigned this Jul 22, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 22, 2024

@llvm/pr-subscribers-clang

Author: Ilya Biryukov (ilya-biryukov)

Changes

This addresses the FIXME in the code. There are tests for the new behavior in a follow up fix for #99877, which also addresses other bugs that prevent exposing the wrong results of ContainsUnexpandedPacks in the outputs of the compiler without crashes.


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

1 Files Affected:

  • (modified) clang/lib/AST/DeclTemplate.cpp (+27-11)
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index 722c7fcf0b0df..f95be88e6c087 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -61,27 +61,43 @@ TemplateParameterList::TemplateParameterList(const ASTContext& C,
 
     bool IsPack = P->isTemplateParameterPack();
     if (const auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(P)) {
-      if (!IsPack && NTTP->getType()->containsUnexpandedParameterPack())
-        ContainsUnexpandedParameterPack = true;
+      if (!IsPack) {
+        if (NTTP->getType()->containsUnexpandedParameterPack())
+          ContainsUnexpandedParameterPack = true;
+        else if (NTTP->hasDefaultArgument() &&
+                 NTTP->getDefaultArgument()
+                     .getArgument()
+                     .containsUnexpandedParameterPack())
+          ContainsUnexpandedParameterPack = true;
+      }
       if (NTTP->hasPlaceholderTypeConstraint())
         HasConstrainedParameters = true;
     } else if (const auto *TTP = dyn_cast<TemplateTemplateParmDecl>(P)) {
-      if (!IsPack &&
-          TTP->getTemplateParameters()->containsUnexpandedParameterPack())
-        ContainsUnexpandedParameterPack = true;
-    } else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(P)) {
-      if (const TypeConstraint *TC = TTP->getTypeConstraint()) {
-        if (TC->getImmediatelyDeclaredConstraint()
-            ->containsUnexpandedParameterPack())
+      if (!IsPack) {
+        if (TTP->getTemplateParameters()->containsUnexpandedParameterPack())
           ContainsUnexpandedParameterPack = true;
+        else if (TTP->hasDefaultArgument() &&
+                 TTP->getDefaultArgument()
+                     .getArgument()
+                     .containsUnexpandedParameterPack())
+          ContainsUnexpandedParameterPack = true;
+      }
+    } else if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(P)) {
+      if (!IsPack && TTP->hasDefaultArgument() &&
+          TTP->getDefaultArgument()
+              .getArgument()
+              .containsUnexpandedParameterPack()) {
+        ContainsUnexpandedParameterPack = true;
+      } else if (const TypeConstraint *TC = TTP->getTypeConstraint();
+                 TC && TC->getImmediatelyDeclaredConstraint()
+                           ->containsUnexpandedParameterPack()) {
+        ContainsUnexpandedParameterPack = true;
       }
       if (TTP->hasTypeConstraint())
         HasConstrainedParameters = true;
     } else {
       llvm_unreachable("unexpected template parameter type");
     }
-    // FIXME: If a default argument contains an unexpanded parameter pack, the
-    // template parameter list does too.
   }
 
   if (HasRequiresClause) {

Comment on lines 68 to 70
NTTP->getDefaultArgument()
.getArgument()
.containsUnexpandedParameterPack())
Copy link
Contributor

Choose a reason for hiding this comment

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

could we have a function - or at least a lambda - that avoid the repetition of that 3 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

…andedPacks

This addresses the FIXME in the code. There are tests for the new
behavior in a follow up fix for llvm#99877, which also addresses other bugs
that prevent exposing the wrong results of `ContainsUnexpandedPacks` in
the outputs of the compiler without crashes.
…nsUnexpandedPacks

Add a helper function to avoid code duplication.
Copy link

github-actions bot commented Jul 23, 2024

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

…nsUnexpandedPacks

Simplify the code and reduce nesting
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

Modulo nitpicking I failed to do earlier this looks great.
Thanks!

…nsUnexpandedPacks

- Rename function
- Use static instead of anonymous namespace
@ilya-biryukov ilya-biryukov merged commit 1c9085e into llvm:main Jul 23, 2024
4 of 5 checks passed
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
…andedPacks (#99880)

Summary:
This addresses the FIXME in the code. There will be tests for the new
behavior in an upcoming #86265, which also addresses other bugs that
prevent exposing the wrong results of `ContainsUnexpandedPacks` in the
outputs of the compiler without crashes.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants