[clang] Implement P2582R1: CTAD from inherited constructors #98788
Conversation
antangelo
requested review from
mizvekov,
cor3ntin,
hokein,
AaronBallman,
erichkeane and
sdkrystian
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: None (antangelo) ChangesThere are two known issues with this initial implementation:
I intend to target clang 20 for merging this patch. Closes #92606 Patch is 53.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/98788.diff 13 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5dc0f8b7e0bbb..b6924f61814fb 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -263,6 +263,8 @@ C++23 Feature Support
- Implemented `P2797R0: Static and explicit object member functions with the same parameter-type-lists <https://wg21.link/P2797R0>`_.
This completes the support for "deducing this".
+- Implemented `P2582R1: Wording for class template argument deduction from inherited constructors <https://wg21.link/P2582R1>`_.
+
C++2c Feature Support
^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index fb52ac804849d..c8e74f32b2ccb 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1957,10 +1957,14 @@ class CXXDeductionGuideDecl : public FunctionDecl {
ExplicitSpecifier ES,
const DeclarationNameInfo &NameInfo, QualType T,
TypeSourceInfo *TInfo, SourceLocation EndLocation,
- CXXConstructorDecl *Ctor, DeductionCandidate Kind)
+ CXXConstructorDecl *Ctor, DeductionCandidate Kind,
+ CXXDeductionGuideDecl *GeneratedFrom,
+ bool IsGeneratedFromInheritedConstructor)
: FunctionDecl(CXXDeductionGuide, C, DC, StartLoc, NameInfo, T, TInfo,
SC_None, false, false, ConstexprSpecKind::Unspecified),
- Ctor(Ctor), ExplicitSpec(ES) {
+ Ctor(Ctor), ExplicitSpec(ES),
+ SourceDeductionGuide(GeneratedFrom,
+ IsGeneratedFromInheritedConstructor) {
if (EndLocation.isValid())
setRangeEnd(EndLocation);
setDeductionCandidateKind(Kind);
@@ -1968,6 +1972,11 @@ class CXXDeductionGuideDecl : public FunctionDecl {
CXXConstructorDecl *Ctor;
ExplicitSpecifier ExplicitSpec;
+ // The deduction guide, if any, that this deduction guide was generated from,
+ // in the case of alias template deduction or CTAD from inherited
+ // constructors. The bool member indicates whether this deduction guide is
+ // generated from an inherited constructor.
+ llvm::PointerIntPair<CXXDeductionGuideDecl *, 1, bool> SourceDeductionGuide;
void setExplicitSpecifier(ExplicitSpecifier ES) { ExplicitSpec = ES; }
public:
@@ -1979,7 +1988,9 @@ class CXXDeductionGuideDecl : public FunctionDecl {
ExplicitSpecifier ES, const DeclarationNameInfo &NameInfo, QualType T,
TypeSourceInfo *TInfo, SourceLocation EndLocation,
CXXConstructorDecl *Ctor = nullptr,
- DeductionCandidate Kind = DeductionCandidate::Normal);
+ DeductionCandidate Kind = DeductionCandidate::Normal,
+ CXXDeductionGuideDecl *SourceDG = nullptr,
+ bool IsGeneratedFromInheritedConstructor = false);
static CXXDeductionGuideDecl *CreateDeserialized(ASTContext &C,
GlobalDeclID ID);
@@ -1999,6 +2010,25 @@ class CXXDeductionGuideDecl : public FunctionDecl {
/// this is an implicit deduction guide.
CXXConstructorDecl *getCorrespondingConstructor() const { return Ctor; }
+ /// Get the deduction guide from which this deduction guide was generated,
+ /// if it was generated as part of alias template deduction or from an
+ /// inherited constructor.
+ CXXDeductionGuideDecl *getSourceDeductionGuide() const {
+ return SourceDeductionGuide.getPointer();
+ }
+
+ void setSourceDeductionGuide(CXXDeductionGuideDecl *DG) {
+ SourceDeductionGuide.setPointer(DG);
+ }
+
+ bool isGeneratedFromInheritedConstructor() const {
+ return SourceDeductionGuide.getInt();
+ }
+
+ void setGeneratedFromInheritedConstructor(bool G = true) {
+ SourceDeductionGuide.setInt(G);
+ }
+
void setDeductionCandidateKind(DeductionCandidate K) {
FunctionDeclBits.DeductionCandidateKind = static_cast<unsigned char>(K);
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 0ea3677355169..da144704957ca 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -4808,6 +4808,10 @@ def note_ovl_candidate_underqualified : Note<
"make %2 equal %1">;
def note_ovl_candidate_substitution_failure : Note<
"candidate template ignored: substitution failure%0%1">;
+def note_ovl_candidate_inherited_constructor_deduction_failure: Note<
+ "candidate template ignored: could not deduce template arguments for %0 from %1%2">;
+def note_ovl_candidate_inherited_constructor_deduction_failure_source: Note<
+ "generated from %0 constructor">;
def note_ovl_candidate_disabled_by_enable_if : Note<
"candidate template ignored: disabled by %0%1">;
def note_ovl_candidate_disabled_by_requirement : Note<
diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp
index 9bb035c07b8ae..e4c6ef256872a 100644
--- a/clang/lib/AST/ASTImporter.cpp
+++ b/clang/lib/AST/ASTImporter.cpp
@@ -3937,14 +3937,16 @@ ExpectedDecl ASTNodeImporter::VisitFunctionDecl(FunctionDecl *D) {
importExplicitSpecifier(Err, Guide->getExplicitSpecifier());
CXXConstructorDecl *Ctor =
importChecked(Err, Guide->getCorrespondingConstructor());
+ CXXDeductionGuideDecl *SourceDG =
+ importChecked(Err, Guide->getSourceDeductionGuide());
if (Err)
return std::move(Err);
if (GetImportedOrCreateDecl<CXXDeductionGuideDecl>(
ToFunction, D, Importer.getToContext(), DC, ToInnerLocStart, ESpec,
- NameInfo, T, TInfo, ToEndLoc, Ctor))
+ NameInfo, T, TInfo, ToEndLoc, Ctor,
+ Guide->getDeductionCandidateKind(), SourceDG,
+ Guide->isGeneratedFromInheritedConstructor()))
return ToFunction;
- cast<CXXDeductionGuideDecl>(ToFunction)
- ->setDeductionCandidateKind(Guide->getDeductionCandidateKind());
} else {
if (GetImportedOrCreateDecl(
ToFunction, D, Importer.getToContext(), DC, ToInnerLocStart,
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index d5c140fd34389..91877a49dcf5e 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -2157,9 +2157,11 @@ CXXDeductionGuideDecl *CXXDeductionGuideDecl::Create(
ASTContext &C, DeclContext *DC, SourceLocation StartLoc,
ExplicitSpecifier ES, const DeclarationNameInfo &NameInfo, QualType T,
TypeSourceInfo *TInfo, SourceLocation EndLocation, CXXConstructorDecl *Ctor,
- DeductionCandidate Kind) {
- return new (C, DC) CXXDeductionGuideDecl(C, DC, StartLoc, ES, NameInfo, T,
- TInfo, EndLocation, Ctor, Kind);
+ DeductionCandidate Kind, CXXDeductionGuideDecl *SourceDG,
+ bool IsGeneratedFromInheritedConstructor) {
+ return new (C, DC) CXXDeductionGuideDecl(
+ C, DC, StartLoc, ES, NameInfo, T, TInfo, EndLocation, Ctor, Kind,
+ SourceDG, IsGeneratedFromInheritedConstructor);
}
CXXDeductionGuideDecl *
@@ -2167,7 +2169,7 @@ CXXDeductionGuideDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID) {
return new (C, ID) CXXDeductionGuideDecl(
C, nullptr, SourceLocation(), ExplicitSpecifier(), DeclarationNameInfo(),
QualType(), nullptr, SourceLocation(), nullptr,
- DeductionCandidate::Normal);
+ DeductionCandidate::Normal, nullptr, false);
}
RequiresExprBodyDecl *RequiresExprBodyDecl::Create(
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index d4a48858ec419..df2b00edaf940 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -10531,6 +10531,40 @@ bool clang::isBetterOverloadCandidate(
auto *Guide1 = dyn_cast_or_null<CXXDeductionGuideDecl>(Cand1.Function);
auto *Guide2 = dyn_cast_or_null<CXXDeductionGuideDecl>(Cand2.Function);
if (Guide1 && Guide2) {
+ // -- F1 and F2 are generated from class template argument deduction
+ // for a class D, and F2 is generated from inheriting constructors
+ // from a base class of D while F1 is not, ...
+ if (Guide1->isImplicit() && Guide2->isImplicit() &&
+ Guide1->isGeneratedFromInheritedConstructor() !=
+ Guide2->isGeneratedFromInheritedConstructor()) {
+ const FunctionProtoType *FPT1 =
+ Guide1->getType()->getAs<FunctionProtoType>();
+ const FunctionProtoType *FPT2 =
+ Guide2->getType()->getAs<FunctionProtoType>();
+ assert(FPT1 && FPT2);
+
+ // ... and for each explicit function argument, the parameters of F1 and
+ // F2 are either both ellipses or have the same type
+ if (FPT1->isVariadic() == FPT2->isVariadic() &&
+ FPT1->getNumParams() == FPT2->getNumParams()) {
+ bool ParamsHaveSameType = true;
+ const auto &A1 = FPT1->getParamTypes();
+ const auto &A2 = FPT2->getParamTypes();
+ for (unsigned I = 0, N = FPT1->getNumParams(); I != N; ++I) {
+ llvm::FoldingSetNodeID ID1, ID2;
+ S.Context.getCanonicalType(A1[I]).Profile(ID1);
+ S.Context.getCanonicalType(A2[I]).Profile(ID2);
+ if (ID1 != ID2) {
+ ParamsHaveSameType = false;
+ break;
+ }
+ }
+
+ if (ParamsHaveSameType)
+ return Guide2->isGeneratedFromInheritedConstructor();
+ }
+ }
+
// -- F1 is generated from a deduction-guide and F2 is not
if (Guide1->isImplicit() != Guide2->isImplicit())
return Guide2->isImplicit();
@@ -11671,6 +11705,39 @@ static void DiagnoseBadDeduction(Sema &S, NamedDecl *Found, Decl *Templated,
return;
}
+ // Errors in deduction guides from inherited constructors
+ // will present as substitution failures in the mapping
+ // partial specialization, so we show a generic diagnostic
+ // in this case.
+ if (auto *DG = dyn_cast<CXXDeductionGuideDecl>(Templated);
+ DG && DG->isGeneratedFromInheritedConstructor()) {
+ CXXDeductionGuideDecl *Source = DG->getSourceDeductionGuide();
+ assert(Source &&
+ "Inherited constructor deduction guides must have a source");
+ auto DeducedRecordType =
+ QualType(cast<ClassTemplateDecl>(DG->getDeducedTemplate())
+ ->getTemplatedDecl()
+ ->getTypeForDecl(),
+ 0);
+ auto InheritedRecordType =
+ QualType(cast<ClassTemplateDecl>(Source->getDeducedTemplate())
+ ->getTemplatedDecl()
+ ->getTypeForDecl(),
+ 0);
+ S.Diag(Templated->getLocation(),
+ diag::note_ovl_candidate_inherited_constructor_deduction_failure)
+ << DeducedRecordType << InheritedRecordType << TemplateArgString;
+
+ CXXConstructorDecl *Ctor = DG->getCorrespondingConstructor();
+ if (Ctor && Ctor->getBeginLoc().isValid())
+ S.Diag(
+ Ctor->getBeginLoc(),
+ diag::
+ note_ovl_candidate_inherited_constructor_deduction_failure_source)
+ << InheritedRecordType;
+ return;
+ }
+
// Format the SFINAE diagnostic into the argument string.
// FIXME: Add a general mechanism to include a PartialDiagnostic *'s
// formatted message in another diagnostic.
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 9296cc66d6918..ec939a2fb718d 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -2746,7 +2746,7 @@ struct ConvertConstructorToDeductionGuideTransform {
}
};
-unsigned getTemplateParameterDepth(NamedDecl *TemplateParam) {
+unsigned getTemplateParameterDepth(const NamedDecl *TemplateParam) {
if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(TemplateParam))
return TTP->getDepth();
if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(TemplateParam))
@@ -2768,7 +2768,7 @@ unsigned getTemplateParameterIndex(NamedDecl *TemplateParam) {
// Find all template parameters that appear in the given DeducedArgs.
// Return the indices of the template parameters in the TemplateParams.
-SmallVector<unsigned> TemplateParamsReferencedInTemplateArgumentList(
+llvm::SmallSet<unsigned, 8> TemplateParamsReferencedInTemplateArgumentList(
const TemplateParameterList *TemplateParamsList,
ArrayRef<TemplateArgument> DeducedArgs) {
struct TemplateParamsReferencedFinder
@@ -2806,17 +2806,19 @@ SmallVector<unsigned> TemplateParamsReferencedInTemplateArgumentList(
}
void Mark(unsigned Depth, unsigned Index) {
if (Index < TemplateParamList->size() &&
- TemplateParamList->getParam(Index)->getTemplateDepth() == Depth)
+ getTemplateParameterDepth(TemplateParamList->getParam(Index)) ==
+ Depth) {
ReferencedTemplateParams.set(Index);
+ }
}
};
TemplateParamsReferencedFinder Finder(TemplateParamsList);
Finder.TraverseTemplateArguments(DeducedArgs);
- SmallVector<unsigned> Results;
+ llvm::SmallSet<unsigned, 8> Results;
for (unsigned Index = 0; Index < TemplateParamsList->size(); ++Index) {
if (Finder.ReferencedTemplateParams[Index])
- Results.push_back(Index);
+ Results.insert(Index);
}
return Results;
}
@@ -3011,9 +3013,12 @@ buildAssociatedConstraints(Sema &SemaRef, FunctionTemplateDecl *F,
Expr *buildIsDeducibleConstraint(Sema &SemaRef,
TypeAliasTemplateDecl *AliasTemplate,
QualType ReturnType,
- SmallVector<NamedDecl *> TemplateParams) {
+ SmallVector<NamedDecl *> TemplateParams,
+ TemplateDecl *DeducingTemplate = nullptr) {
ASTContext &Context = SemaRef.Context;
// Constraint AST nodes must use uninstantiated depth.
+ assert(!DeducingTemplate || DeducingTemplate->getTemplateDepth() ==
+ AliasTemplate->getTemplateDepth());
if (auto *PrimaryTemplate =
AliasTemplate->getInstantiatedFromMemberTemplate();
PrimaryTemplate && TemplateParams.size() > 0) {
@@ -3046,19 +3051,21 @@ Expr *buildIsDeducibleConstraint(Sema &SemaRef,
Context.DeclarationNames.getCXXDeductionGuideName(AliasTemplate));
};
+ TemplateDecl *TD = DeducingTemplate ? DeducingTemplate : AliasTemplate;
+
SmallVector<TypeSourceInfo *> IsDeducibleTypeTraitArgs = {
Context.getTrivialTypeSourceInfo(
Context.getDeducedTemplateSpecializationType(
- TemplateName(AliasTemplate), /*DeducedType=*/QualType(),
+ TemplateName(TD), /*DeducedType=*/QualType(),
/*IsDependent=*/true)), // template specialization type whose
// arguments will be deduced.
Context.getTrivialTypeSourceInfo(
ReturnType), // type from which template arguments are deduced.
};
- return TypeTraitExpr::Create(
- Context, Context.getLogicalOperationType(), AliasTemplate->getLocation(),
- TypeTrait::BTT_IsDeducible, IsDeducibleTypeTraitArgs,
- AliasTemplate->getLocation(), /*Value*/ false);
+ return TypeTraitExpr::Create(Context, Context.getLogicalOperationType(),
+ TD->getLocation(), TypeTrait::BTT_IsDeducible,
+ IsDeducibleTypeTraitArgs, TD->getLocation(),
+ /*Value*/ false);
}
std::pair<TemplateDecl *, llvm::ArrayRef<TemplateArgument>>
@@ -3090,12 +3097,59 @@ getRHSTemplateDeclAndArgs(Sema &SemaRef, TypeAliasTemplateDecl *AliasTemplate) {
return {Template, AliasRhsTemplateArgs};
}
+// Build the type for a deduction guide generated from an inherited constructor
+// [over.match.class.deduct]p1.10:
+// ... the set contains the guides of A with the return type R
+// of each guide replaced with `typename CC<R>::type` ...
+std::pair<TypeSourceInfo *, QualType>
+buildInheritedConstructorDeductionGuideType(
+ Sema &SemaRef, TypeSourceInfo *DerivedClassMapperType,
+ TemplateDecl *DeducingTemplate, TypeSourceInfo *SourceGuideTSI) {
+ auto &Context = SemaRef.Context;
+ const auto *FPT = SourceGuideTSI->getType()->getAs<FunctionProtoType>();
+ assert(FPT && "Source Guide type should be a FunctionProtoType");
+
+ // This substitution can fail in cases where the source return type
+ // is not dependent and the derived class is not deducible
+ Sema::SFINAETrap Trap(SemaRef);
+
+ MultiLevelTemplateArgumentList Args;
+ Args.addOuterTemplateArguments(DeducingTemplate,
+ TemplateArgument(FPT->getReturnType()), false);
+ Args.addOuterRetainedLevels(DeducingTemplate->getTemplateDepth());
+ TypeSourceInfo *ReturnTypeTSI =
+ SemaRef.SubstType(DerivedClassMapperType, Args,
+ DeducingTemplate->getBeginLoc(), DeclarationName());
+ if (!ReturnTypeTSI || Trap.hasErrorOccurred())
+ return {nullptr, QualType()};
+ const QualType &ReturnType = ReturnTypeTSI->getType();
+
+ TypeLocBuilder TLB;
+ TLB.pushFullCopy(ReturnTypeTSI->getTypeLoc());
+
+ QualType FT = Context.getFunctionType(ReturnType, FPT->getParamTypes(),
+ FPT->getExtProtoInfo());
+ FunctionProtoTypeLoc NewTL = TLB.push<FunctionProtoTypeLoc>(FT);
+ const auto &TL = SourceGuideTSI->getTypeLoc().getAs<FunctionProtoTypeLoc>();
+ NewTL.setLocalRangeBegin(TL.getLocalRangeBegin());
+ NewTL.setLParenLoc(TL.getLParenLoc());
+ NewTL.setRParenLoc(TL.getRParenLoc());
+ NewTL.setExceptionSpecRange(TL.getExceptionSpecRange());
+ NewTL.setLocalRangeEnd(TL.getLocalRangeEnd());
+ for (unsigned I = 0, E = NewTL.getNumParams(); I != E; ++I)
+ NewTL.setParam(I, TL.getParam(I));
+
+ TypeSourceInfo *TSI = TLB.getTypeSourceInfo(Context, FT);
+ return {TSI, ReturnType};
+}
+
// Build deduction guides for a type alias template from the given underlying
// deduction guide F.
-FunctionTemplateDecl *
-BuildDeductionGuideForTypeAlias(Sema &SemaRef,
- TypeAliasTemplateDecl *AliasTemplate,
- FunctionTemplateDecl *F, SourceLocation Loc) {
+FunctionTemplateDecl *BuildDeductionGuideForTypeAlias(
+ Sema &SemaRef, TypeAliasTemplateDecl *AliasTemplate,
+ FunctionTemplateDecl *F, SourceLocation Loc,
+ TemplateDecl *DeducingTemplate = nullptr,
+ TypeSourceInfo *DerivedClassMapperType = nullptr) {
LocalInstantiationScope Scope(SemaRef);
Sema::InstantiatingTemplate BuildingDeductionGuides(
SemaRef, AliasTemplate->getLocation(), F,
@@ -3103,6 +3157,9 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
if (BuildingDeductionGuides.isInvalid())
return nullptr;
+ if (!DeducingTemplate)
+ DeducingTemplate = AliasTemplate;
+
auto &Context = SemaRef.Context;
auto [Template, AliasRhsTemplateArgs] =
getRHSTemplateDeclAndArgs(SemaRef, AliasTemplate);
@@ -3268,8 +3325,17 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
Sema::CodeSynthesisContext::BuildingDeductionGuides)) {
auto *GG = cast<CXXDeductionGuideDecl>(FPrime);
- Expr *IsDeducible = buildIsDeducibleConstraint(
- SemaRef, AliasTemplate, FPrime->getReturnType(), FPrimeTemplateParams);
+ TypeSourceInfo *TSI = GG->getTypeSourceInfo();
+ QualType ReturnType = FPrime->getReturnType();
+ if (DerivedClassMapperType)
+ std::tie(TSI, ReturnType) = buildInheritedConstructorDeductionGuideType(
+ SemaRef, DerivedClassMapperType, DeducingTemplate, TSI);
+ if (!TSI)
+ return nullptr;
+
+ Expr *IsDeducible =
+ buildIsDeducibleConstraint(SemaRef, AliasTemplate, ReturnType,
+ FPrimeTemplateParams, DeducingTemplate);
Expr *RequiresClause =
buildAssociatedConstraints(SemaRef, F, AliasTemplate, DeduceResults,
FirstUndeducedParamIdx, IsDeducible);
@@ -3281,27 +3347,37 @@ BuildDeductionGuideForTypeAlias(Sema &SemaRef,
AliasTemplate->getTemplateParameters()->getRAngleLoc(),
/*RequiresClause=*/RequiresClause);
auto *Result = cast<FunctionTemplateDecl>(buildDeductionGuide(
- SemaRef, AliasTemplate, FPrimeTemplateParamList,
- GG->getCorrespondingConstructor(), GG->getExplicitSpecifier(),
- GG->getTypeSourceInfo(), AliasTemp...
[truncated]
|
clang/lib/AST/DeclCXX.cpp
Outdated
| } | ||
|
|
||
| CXXDeductionGuideDecl * | ||
| CXXDeductionGuideDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID) { | ||
| return new (C, ID) CXXDeductionGuideDecl( | ||
| C, nullptr, SourceLocation(), ExplicitSpecifier(), DeclarationNameInfo(), | ||
| QualType(), nullptr, SourceLocation(), nullptr, | ||
| DeductionCandidate::Normal); | ||
| DeductionCandidate::Normal, nullptr, false); |
There was a problem hiding this comment.
| DeductionCandidate::Normal, nullptr, false); | |
| DeductionCandidate::Normal, /*SourceDG=*/nullptr, /*IsGeneratedFromInheritedConstructor =*/false); |
This is based on bugprone argument comment convention.
There was a problem hiding this comment.
Thanks, I've updated those arguments as you suggested. Do you want me to also update the others, or just those two new ones?
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks for implementing it! I haven't looked into the implementation details yet. Per C++ over.match.class.deduct#1.10:
Let's use the following example to illustrate: template <typename T1> struct B {
B(T1);
};
template <typename T2> struct C : public B<T2> {
using B<T2>::B;
};Here, Steps:
The we get the final deduction guide for C: |
That looks correct to my understanding of the standard. |
There was a problem hiding this comment.
I think that this looks generally good
A few comments
- can you give a description if what needs to be done in subsequent PRs? (maybe cxx_status.html should say "partial"
- can you add a release notes
- can you add PCH or module tests for the changes to ASTReader/ASTWriter?
| // We omit the deducible constraint for inherited constructor deduction | ||
| // guides because they would take precedence over the derived class' own | ||
| // deduction guides due to [over.match.best.general]p2.5 and | ||
| // [temp.func.order]p6.4 If the alias were not deducible in this case, the | ||
| // deduction guide would already not be deducible due to the partial | ||
| // specialization `CC<>` failing substitution. |
There was a problem hiding this comment.
Did we decide this is correct? Should it not be a conjunction?
There was a problem hiding this comment.
Not yet, there's been no response from the CWG on the issue. The current implementation follows the suggestion outlined in that issue, and I think it's fine as is. However, we should probably reference this issue in a comment for clarity.
You can also check our related discussion.
| @@ -1398,5 +1807,30 @@ void Sema::DeclareImplicitDeductionGuides(TemplateDecl *Template, | |||
| ->getTemplatedDecl()) | |||
| ->setDeductionCandidateKind(DeductionCandidate::Copy); | |||
|
|
|||
| CXXRecordDecl *TemplatedDecl = Pattern->getTemplatedDecl(); | |||
| if (getLangOpts().CPlusPlus23 && TemplatedDecl->hasDefinition()) { | |||
There was a problem hiding this comment.
Is that something we should support in C++20 as an extension?
There was a problem hiding this comment.
Given the uncertainty with open CWG issues and the potential breakage of code on earlier standards versions, I think it's less risky to keep it constrained to C++23 for now. It's probably worth revisiting in the future, though.
| // We omit the deducible constraint for inherited constructor deduction | ||
| // guides because they would take precedence over the derived class' own | ||
| // deduction guides due to [over.match.best.general]p2.5 and | ||
| // [temp.func.order]p6.4 If the alias were not deducible in this case, the | ||
| // deduction guide would already not be deducible due to the partial | ||
| // specialization `CC<>` failing substitution. |
There was a problem hiding this comment.
Not yet, there's been no response from the CWG on the issue. The current implementation follows the suggestion outlined in that issue, and I think it's fine as is. However, we should probably reference this issue in a comment for clarity.
You can also check our related discussion.
- Rename SourceKind to SourceDeductionGuideKind and add a clarifying comment - Add modules test case for source deduction guide AST Reader/Writer changes - Update ParamsHaveSameType check to use all_of_zip and restrict lambda capture - Update comment to replace mapping terminology to better refer to the return type partial specialization - Refactor deduction guide deduced template type to a lambda - Remove unneeded check for invalid Ctor begin loc - Update standards comments to refer to C++23 - Fix a bugprone argument convention comment
|
Thanks for the reviews!
The remaining tasks are:
I have updated the cxx_status to partial, and added a note about the unsupported depdendent using-declarators. This aligns with the status for alias template deduction, which is partial due to the feature macro not being set. I presume the issue of deduction guides declared after the first lookup of deduction guides would be fixed in both features at the same time, either in this patch set if it is fixed in upstream first, or after this patch set as a followup.
There should already be a clang release note, are there any other spots besides ReleaseNotes.rst and cxx_status.html that need updating?
I have added module tests that cover the feature. |
Although there are FIXMEs in the code, could you also file a GitHub issue for better tracking?
Yes, the missing-after-first-lookup issue is already tracked in #103016, and it also affects alias template deduction (it’s the only blocker remaining to mark alias template deduction as a completed feature). We can address this as a follow-up, and I think we should resolve it before the next Clang 20 release. |
clang/include/clang/AST/DeclCXX.h
Outdated
| /// Get the deduction guide from which this deduction guide was generated, | ||
| /// if it was generated as part of alias template deduction or from an | ||
| /// inherited constructor. | ||
| CXXDeductionGuideDecl *getSourceDeductionGuide() const { |
There was a problem hiding this comment.
| CXXDeductionGuideDecl *getSourceDeductionGuide() const { | |
| const CXXDeductionGuideDecl *getSourceDeductionGuide() const { |
Would that break anything?
There was a problem hiding this comment.
Yeah, we should be returning a const pointer from a const member function; we can add a non-const member function overload to return a non-const pointer if we need one.
| std::string CCTemplateDeclName = | ||
| (Twine("__ctad_CC_") + BaseTD->getName() + "_to_" + Template->getName() + | ||
| "_" + Twine(BaseIdx)) | ||
| .str(); | ||
| IdentifierInfo *CCTemplateII = &Context.Idents.get(CCTemplateDeclName); | ||
| CXXRecordDecl *CCTemplateRD = CXXRecordDecl::Create( |
There was a problem hiding this comment.
Do we actually need a name here (or could the record be anonymous)?
There was a problem hiding this comment.
I initially tried it as anonymous, and it worked in normal use, but I experienced some ICE crashes while debugging/dumping types that included it. I am not sure which internal assumptions are broken by this, if any, given that anonymous templates are not supported by the language, which is why I ended up naming it.
I also imagine that looking this up by its name will be the most efficient way to retrieve the CC and alias types again when we implement handling for deduction guides declared after the first use (since we won't be recreating these AST nodes, and storing references to them anywhere else seems too wasteful).
| // expected-note {{implicit deduction guide declared as 'template <NoPointers<> T, typename V> ExplicitDerived(V) -> typename __ctad_CC_ExplicitBase_to_ExplicitDerived_0<ExplicitBase<T>>::type'}} \ | ||
| // expected-note {{candidate template ignored: could not match 'ExplicitBase<T>' against 'const char *'}} \ | ||
| // expected-note {{candidate template ignored: could not deduce template arguments for 'ExplicitDerived<T>' from 'ExplicitBase<T>' [with T = const char *]}} \ | ||
| // expected-note {{implicit deduction guide declared as 'template <NoPointers<> T> ExplicitDerived(ExplicitBase<T>) -> typename __ctad_CC_ExplicitBase_to_ExplicitDerived_0<ExplicitBase<T>>::type'}} |
There was a problem hiding this comment.
This is not the best diagnostics.
Could we detect implicit deduction guides and
show something like
<implicit deduction guide for X inherited from Y>
note :Ydeclared here
There was a problem hiding this comment.
By <implicit deduction guide for X inherited from Y> do you mean to show the declaration for the original deduction guide on the base instead of the transformed guide on the derived class? (Or do you mean to omit showing the declaration entirely for inherited guides in favor of showing just the base declaration?)
There was a problem hiding this comment.
I have updated the diagnostics so that we print the original source deduction guide, following the source pointer, and for each source pointer we follow I also now print the location that the relevant constructor is inherited.
clang/include/clang/AST/DeclCXX.h
Outdated
| /// Get the deduction guide from which this deduction guide was generated, | ||
| /// if it was generated as part of alias template deduction or from an | ||
| /// inherited constructor. | ||
| CXXDeductionGuideDecl *getSourceDeductionGuide() const { |
There was a problem hiding this comment.
Yeah, we should be returning a const pointer from a const member function; we can add a non-const member function overload to return a non-const pointer if we need one.
| if (FPT1->isVariadic() == FPT2->isVariadic() && | ||
| FPT1->getNumParams() == FPT2->getNumParams()) { |
There was a problem hiding this comment.
This seems wrong compared to the comment -- the comment says we should be looking at each explicit function argument, but this predicate only handles the case where both functions are variadic and have the same number of params.
Am I missing something?
There was a problem hiding this comment.
To my understanding of the implications of the wording, if they do not have the same number of parameters, then at least one of the parameters must not have the same type (since the corresponding parameter in the other function wouldn't exist). If this is the case, then we don't need to do the work of checking all the parameters individually.
The variadic check is that the functions are either both variadic, or both not variadic, for handling of the ellipses parameters.
There was a problem hiding this comment.
Is there a reason we can't just check the types of the functions here instead? I would assume their prototypes to compare equal. That said, the wording of parameters are both ellipses or have the same type seems intentionally designed for the purpose of excluding the return type (or, perhaps reference/cv specifiers?)?
If so, I think I'd rather a variant of hasSameType that for something like, hasSameParameterList. At that point, we could have IT check for similar variadics as well. We probably want to make sure that (int, ...) works too.
Also, I don't think the initial check is right? Shouldn't that be an 'or' there instead of '&&'? If both are NumParams == 0 && isVariadic, then we want to hit this condition as well.
There was a problem hiding this comment.
The return types will not be equal as the inherited deduction guides will have some typename CC<R>::type as the return type, while non-inherited deduction guides will just have a template specialization type for the class template.
If both functions are variadic and have getNumParams() == 0, then that boolean expression evaluates to true right? Can you elaborate on what you think the behavior there should be? I may be misunderstanding what the standards wording on this part is asking.
There was a problem hiding this comment.
We could assert that getNumNonObjectParams is false (I don't think we can get explicit objects anywhere, right? Otherwise, the code looks correct, but I agree that something like a "haveSameNonObjectParameters" function would improve readability
clang/lib/Sema/SemaOverload.cpp
Outdated
| // will manifest as substitution failures in the return type | ||
| // partial specialization, so we show a generic diagnostic | ||
| // in this case. | ||
| if (auto *DG = dyn_cast<CXXDeductionGuideDecl>(Templated); |
There was a problem hiding this comment.
| if (auto *DG = dyn_cast<CXXDeductionGuideDecl>(Templated); | |
| if (const auto *DG = dyn_cast<CXXDeductionGuideDecl>(Templated); |
and handle const correctness further down as well.
| TypeSourceInfo *buildInheritedConstructorDeductionGuideType( | ||
| Sema &SemaRef, const InheritedConstructorDeductionInfo &Info, | ||
| TypeSourceInfo *SourceGuideTSI) { | ||
| auto &Context = SemaRef.Context; |
There was a problem hiding this comment.
| auto &Context = SemaRef.Context; | |
| ASTContext &Context = SemaRef.Context; |
| NewTL.setRParenLoc(TL.getRParenLoc()); | ||
| NewTL.setExceptionSpecRange(TL.getExceptionSpecRange()); | ||
| NewTL.setLocalRangeEnd(TL.getLocalRangeEnd()); | ||
| for (unsigned I = 0, E = NewTL.getNumParams(); I != E; ++I) |
There was a problem hiding this comment.
Do we have to do something special here to handle a variadic?
There was a problem hiding this comment.
I don't believe so, from the comment on FunctionProtoType it seems the source location for ellipses is stored in the type itself, and there isn't any mention of them in FunctionProtoTypeLoc
llvm-project/clang/include/clang/AST/Type.h
Lines 5111 to 5112 in 476b208
I have added a C-style variadic test case as well to verify that they work as expected.
| } | ||
| } | ||
|
|
||
| // Check if a template is deducible as per [dcl.type.simple]p3 | ||
| static bool IsDeducibleTemplate(TemplateDecl *TD) { |
There was a problem hiding this comment.
| static bool IsDeducibleTemplate(TemplateDecl *TD) { | |
| static bool IsDeducibleTemplate(const TemplateDecl *TD) { |
and threaded through the rest of the function?
| if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(Param)) { | ||
| TTP->removeDefaultArgument(); | ||
| } else if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param)) { | ||
| NTTP->removeDefaultArgument(); | ||
| } else if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(Param)) { | ||
| TTP->removeDefaultArgument(); | ||
| } |
There was a problem hiding this comment.
| if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(Param)) { | |
| TTP->removeDefaultArgument(); | |
| } else if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param)) { | |
| NTTP->removeDefaultArgument(); | |
| } else if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(Param)) { | |
| TTP->removeDefaultArgument(); | |
| } | |
| if (auto *TTP = dyn_cast<TemplateTypeParmDecl>(Param)) | |
| TTP->removeDefaultArgument(); | |
| else if (auto *NTTP = dyn_cast<NonTypeTemplateParmDecl>(Param)) | |
| NTTP->removeDefaultArgument(); | |
| else if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(Param)) | |
| TTP->removeDefaultArgument(); |
clang/lib/Sema/SemaOverload.cpp
Outdated
| CXXConstructorDecl *Ctor = DG->getCorrespondingConstructor(); | ||
| if (Ctor) | ||
| S.Diag( | ||
| Ctor->getBeginLoc(), | ||
| diag:: | ||
| note_ovl_candidate_inherited_constructor_deduction_failure_source) | ||
| << InheritedRecordType; | ||
| return; |
There was a problem hiding this comment.
We already printed the type in the previous note, and we should be pointing to the source location of the constructor anyway, so we can omit that.
Would it also be worth pointing out that this is an implicit deduction guide?
There was a problem hiding this comment.
I'm using the source location of the inherited constructor deduction guide to store the location of the using decl that inherits the base constructors (as it is otherwise difficult to pass this information around), so the notes for deduction guide failure will point there instead of at the source constructor (or deduction guide in the explicit case).
I have added additional diagnostics that print the source deduction guide declaration (with the source deduction guide's location) for both implicit and explicit guides when we note the deduction guide declarations for implicit guides as a workaround, which also removes the need for the above note by extension.
…rom an inherited DG PR feedback addressed: - Const correctness in CXXDeductionGuideDecl ASTNode and throughout added code - Add FIXME for not emitting diagnostic in case where CC type substitution fails - When emitting notes for deduction guide declarations, follow the source pointer for inherited deduction guides so the first source guide is shown instead of the inherited guide, and emit a note showing the point(s) that the constructors are inherited - Refactor buildInheritedConstructorDeductionGuideType to return the ReturnType directly - Add test for C-style variadic constructor - Replace some uses of 'auto' with the full type - Update if/else-if formatting
| FReturnType = ET->getNamedType()->getAs<TemplateSpecializationType>(); | ||
| assert(FReturnType && "expected to see a return type"); | ||
| ArrayRef<TemplateArgument> FTemplateArgs = | ||
| getTemplateArgumentsFromDeductionGuideReturnType( |
There was a problem hiding this comment.
@hokein I would like to re-request your review for these new changes to BuildDeductionGuideForTypeAlias to handle declaring alias and inherited deduction guides from an inherited deduction guide as the source (e.g. in cases with deeper class hierarchies or aliased class templates that inherit deduction guides).
| // Represents the relationship between this deduction guide and the | ||
| // deduction guide that it was generated from (or lack thereof). | ||
| // See the SourceDeductionGuide member for more details. | ||
| enum class SourceDeductionGuideKind : unsigned char { |
There was a problem hiding this comment.
| enum class SourceDeductionGuideKind : unsigned char { | |
| enum class SourceDeductionGuideKind : uint8_t { |
I thought I saw elsewhere we do this reasonably consistently? Else, feel free to ignore.
| } | ||
|
|
||
| void setSourceDeductionGuide(CXXDeductionGuideDecl *DG) { | ||
| SourceDeductionGuide.setPointer(DG); |
There was a problem hiding this comment.
Do we want to assert for !DG?
There was a problem hiding this comment.
I opted not to assert here and in setSourceDeductionGuideKind because it makes the ASTReader changes more readable (which otherwise would have to do a check on at least one of these fields before setting it on the incoming CXXDeductionGuideDecl node, but maybe that's preferable).
| } | ||
|
|
||
| void setSourceDeductionGuideKind(SourceDeductionGuideKind SK) { | ||
| SourceDeductionGuide.setInt(SK); |
There was a problem hiding this comment.
PERHAPS here too? Assert SK == None?
| SourceDeductionGuideKind SourceKind) { | ||
| return new (C, DC) CXXDeductionGuideDecl( | ||
| C, DC, StartLoc, ES, NameInfo, T, TInfo, EndLocation, Ctor, Kind, | ||
| TrailingRequiresClause, GeneratedFrom, SourceKind); | ||
| } | ||
|
|
||
| CXXDeductionGuideDecl * | ||
| CXXDeductionGuideDecl::CreateDeserialized(ASTContext &C, GlobalDeclID ID) { | ||
| return new (C, ID) CXXDeductionGuideDecl( |
There was a problem hiding this comment.
For Exprs we usually have a CreateEmpty that has an empty/empty-ish private constructor associated with it. or something like that.... this is very much making me wish we had the same for this one.
| if (FPT1->isVariadic() == FPT2->isVariadic() && | ||
| FPT1->getNumParams() == FPT2->getNumParams()) { |
There was a problem hiding this comment.
Is there a reason we can't just check the types of the functions here instead? I would assume their prototypes to compare equal. That said, the wording of parameters are both ellipses or have the same type seems intentionally designed for the purpose of excluding the return type (or, perhaps reference/cv specifiers?)?
If so, I think I'd rather a variant of hasSameType that for something like, hasSameParameterList. At that point, we could have IT check for similar variadics as well. We probably want to make sure that (int, ...) works too.
Also, I don't think the initial check is right? Shouldn't that be an 'or' there instead of '&&'? If both are NumParams == 0 && isVariadic, then we want to hit this condition as well.
| @@ -942,34 +946,145 @@ getRHSTemplateDeclAndArgs(Sema &SemaRef, TypeAliasTemplateDecl *AliasTemplate) { | |||
| return {Template, AliasRhsTemplateArgs}; | |||
| } | |||
|
|
|||
| struct InheritedConstructorDeductionInfo { | |||
| // Class template for which we are declaring deduction guides | |||
| // This is `C` in the standard wording | |||
There was a problem hiding this comment.
Comments are supposed to end in full-stops, so I think there is a sprinkling of . required here/elsewhere here (other than standards quotes).
| Args.addOuterTemplateArguments(Info.DerivedClassTemplate, | ||
| TemplateArgument(FPT->getReturnType()), | ||
| /*Final=*/false); | ||
| Args.addOuterRetainedLevels(Info.DerivedClassTemplate->getTemplateDepth()); |
There was a problem hiding this comment.
As we sure this is sufficient and shouldn't be calling the getInstantiationArgs? It might need help with Deduction guides, but is meant to get the full depth of the deduction guide. But OuterRetainedLevels means it won't fully instantiate (and those levels will just not fill anything in). Though is that perhaps what is intended here?
|
@antangelo @hokein What's the status here? Thanks! |
|
I'm still working on addressing the remaining comments, primarily the one from Erich about insufficient handling of nested templates. There are some issues when the derived class is in a nested template, and the base specifier depends on the outer template arguments, where the CC type we create has arguments substituted at the wrong levels when used in a deduction guide. I haven't had time to debug this fully, but my suspicion is that it has something to do with placing the CC template inside the outer template specialization without it being defined in the outer template pattern (but this suspicion may be wrong). I don't know if I'll be able to fix these issues before clang 20 branches. The best way forward might be for me to:
|
|
Both 1 and 2 seems like good options @hokein @erichkeane |
I like 1, but not really 2, I think we should wait for this to be reasonably complete before merging it, particularly this close to the release. I'd probably be ok with 2 if we wait a few weeks until after the release, so that we can deal with the fallout with less stress, and fix it before Clang21 release. |
|
I have split the source deduction guide tracking into #123875, and will continue working on fixing the remaining issues here to land P2582R1 for clang 21. |
+1. Rushing a new feature just before the release increases the risk of issues and adds unnecessary stress. I believe we should find a way to land this patch, even if it’s not fully complete (it has been pending for months). Landing it after the release cut seems like a good approach. This would allow for incremental development while giving us more time to address any fallout before clang 21. |
|
@antangelo @erichkeane @hokein Should we try to land this now? |
There are two known issues with this initial implementation:
using Derived::Base::Base;. These using declarations in general are unresolved, so it is difficult to look them up and match them to the corresponding base before instantiation.I intend to target clang 20 for merging this patch.
Closes #92606