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] Fix CTAD not respect default template arguments that were added after the definition. #75569

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

hokein
Copy link
Collaborator

@hokein hokein commented Dec 15, 2023

Fixes #69987

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 15, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 15, 2023

@llvm/pr-subscribers-clang

Author: Haojian Wu (hokein)

Changes

Fixes #69987


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) clang/lib/Sema/SemaTemplate.cpp (+13-9)
  • (modified) clang/test/SemaTemplate/ctad.cpp (+10)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f5ae6bb8925202..be826e6a44bfc7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -685,6 +685,8 @@ Bug Fixes in This Version
   (`#62157 <https://github.com/llvm/llvm-project/issues/62157>`_) and
   (`#64885 <https://github.com/llvm/llvm-project/issues/64885>`_) and
   (`#65568 <https://github.com/llvm/llvm-project/issues/65568>`_)
+- Fix an issue where clang doesn't respect detault template arguments that
+  are added in a later redeclaration for CTAD. (#69987 <https://github.com/llvm/llvm-project/issues/69987>`_)
 
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index f10abeaba0d451..450a1a1db0ba86 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1824,6 +1824,15 @@ static void SetNestedNameSpecifier(Sema &S, TagDecl *T,
     T->setQualifierInfo(SS.getWithLocInContext(S.Context));
 }
 
+// Returns the template parameter list with all default template argument
+// information.
+static TemplateParameterList *GetTemplateParameterList(TemplateDecl *TD) {
+  // Make sure we get the template parameter list from the most
+  // recent declaration, since that is the only one that is guaranteed to
+  // have all the default template argument information.
+  return cast<TemplateDecl>(TD->getMostRecentDecl())->getTemplateParameters();
+}
+
 DeclResult Sema::CheckClassTemplate(
     Scope *S, unsigned TagSpec, TagUseKind TUK, SourceLocation KWLoc,
     CXXScopeSpec &SS, IdentifierInfo *Name, SourceLocation NameLoc,
@@ -2062,7 +2071,7 @@ DeclResult Sema::CheckClassTemplate(
       CheckTemplateParameterList(
           TemplateParams,
           PrevClassTemplate
-              ? PrevClassTemplate->getMostRecentDecl()->getTemplateParameters()
+              ? GetTemplateParameterList(PrevClassTemplate)
               : nullptr,
           (SS.isSet() && SemanticContext && SemanticContext->isRecord() &&
            SemanticContext->isDependentContext())
@@ -2298,7 +2307,7 @@ struct ConvertConstructorToDeductionGuideTransform {
     //    -- The template parameters are the template parameters of the class
     //       template followed by the template parameters (including default
     //       template arguments) of the constructor, if any.
-    TemplateParameterList *TemplateParams = Template->getTemplateParameters();
+    TemplateParameterList *TemplateParams = GetTemplateParameterList(Template);
     if (FTD) {
       TemplateParameterList *InnerParams = FTD->getTemplateParameters();
       SmallVector<NamedDecl *, 16> AllParams;
@@ -2424,7 +2433,7 @@ struct ConvertConstructorToDeductionGuideTransform {
       Params.push_back(NewParam);
     }
 
-    return buildDeductionGuide(Template->getTemplateParameters(), nullptr,
+    return buildDeductionGuide(GetTemplateParameterList(Template), nullptr,
                                ExplicitSpecifier(), TSI, Loc, Loc, Loc);
   }
 
@@ -5956,12 +5965,7 @@ bool Sema::CheckTemplateArgumentList(
   // template.
   TemplateArgumentListInfo NewArgs = TemplateArgs;
 
-  // Make sure we get the template parameter list from the most
-  // recent declaration, since that is the only one that is guaranteed to
-  // have all the default template argument information.
-  TemplateParameterList *Params =
-      cast<TemplateDecl>(Template->getMostRecentDecl())
-          ->getTemplateParameters();
+  TemplateParameterList *Params = GetTemplateParameterList(Template);
 
   SourceLocation RAngleLoc = NewArgs.getRAngleLoc();
 
diff --git a/clang/test/SemaTemplate/ctad.cpp b/clang/test/SemaTemplate/ctad.cpp
index 4d836839d8c346..388ed7d4cced18 100644
--- a/clang/test/SemaTemplate/ctad.cpp
+++ b/clang/test/SemaTemplate/ctad.cpp
@@ -44,3 +44,13 @@ namespace Access {
   };
   D z = {Z(), {}};
 }
+
+namespace GH69987 {
+template<class> struct X {};
+template<class = void> struct X;
+X x;
+
+template<class T, class B> struct Y { Y(T); };
+template<class T, class B=void> struct Y ;
+Y y(1);
+};

Copy link

github-actions bot commented Dec 15, 2023

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

@@ -685,6 +685,8 @@ Bug Fixes in This Version
(`#62157 <https://github.com/llvm/llvm-project/issues/62157>`_) and
(`#64885 <https://github.com/llvm/llvm-project/issues/64885>`_) and
(`#65568 <https://github.com/llvm/llvm-project/issues/65568>`_)
- Fix an issue where clang doesn't respect detault template arguments that
are added in a later redeclaration for CTAD. (#69987 <https://github.com/llvm/llvm-project/issues/69987>`_)
Copy link
Contributor

Choose a reason for hiding this comment

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

A little nit to align the style

Suggested change
are added in a later redeclaration for CTAD. (#69987 <https://github.com/llvm/llvm-project/issues/69987>`_)
are added in a later redeclaration for CTAD.
Fixes (#69987 <https://github.com/llvm/llvm-project/issues/69987>`_)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

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.

LGTM, thanks

@hokein
Copy link
Collaborator Author

hokein commented Dec 18, 2023

Thanks for the review.

@hokein hokein merged commit 42239d2 into llvm:main Dec 18, 2023
5 checks passed
@hokein hokein deleted the ctad-default-args branch December 18, 2023 11:37
@shafik
Copy link
Collaborator

shafik commented Dec 19, 2023

Can we please add more detailed summaries, we want the git log to be sufficient for triaging various issues without having to dig into each commit individually.

(SS.isSet() && SemanticContext && SemanticContext->isRecord() &&
SemanticContext->isDependentContext())
? TPC_ClassTemplateMember
: TUK == TUK_Friend ? TPC_FriendClassTemplate : TPC_ClassTemplate,
: TUK == TUK_Friend ? TPC_FriendClassTemplate
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like an unrelated change and it looks funny.

mizvekov added a commit that referenced this pull request Jun 4, 2024
This removes a workaround for template template arguments pointing to
older template declarations, which don't have the most recent default
argument definition.

The removed workaround was introduced in 1abacfc,
but #75569 introduced a
better fix which is more comprehensive and doesn't require rebuilding
TemplateNames.
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.

Default template arguments added in later redeclaration not considered for CTAD
5 participants