[clang] Implement CWG2398 provisional TTP matching to class templates #94981
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mizvekov
requested review from
zygoloid,
rjmccall,
cor3ntin,
hokein,
erichkeane and
sdkrystian
llvmbot
added
clang
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-modules Author: Matheus Izvekov (mizvekov) ChangesThis extends default argument deduction to cover class templates as well, and also applies outside of partial ordering, adding to the provisional wording introduced in #89807. This solves some ambuguity introduced in P0522 regarding how template template parameters are partially ordered, and should reduce the negative impact of enabling Given the following example: template <class T1, class T2 = float> struct A;
template <class T3> struct B;
template <template <class T4> class TT1, class T5> struct B<TT1<T5>>; // #<!-- -->1
template <class T6, class T7> struct B<A<T6, T7>>; // #<!-- -->2
template struct B<A<int>>;Prior to P0522, As the consequences are not restricted to partial ordering, the following code becomes valid: template<class T, class U> struct A {};
A<int, float> v;
template<template<class> class TT> void f(TT<int>);
// OK: TT picks 'float' as the default argument for the second parameter.
void g() { f(v); }Also, since 'f' deduced from Since this changes provisional implementation of CWG2398 which has not been released yet, and already contains a changelog entry, we don't provide a changelog entry here. Patch is 73.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94981.diff 25 Files Affected:
diff --git a/clang-tools-extra/clangd/DumpAST.cpp b/clang-tools-extra/clangd/DumpAST.cpp
index 9a525efb938e8..e605f82e91fe4 100644
--- a/clang-tools-extra/clangd/DumpAST.cpp
+++ b/clang-tools-extra/clangd/DumpAST.cpp
@@ -187,6 +187,7 @@ class DumpVisitor : public RecursiveASTVisitor<DumpVisitor> {
TEMPLATE_KIND(SubstTemplateTemplateParm);
TEMPLATE_KIND(SubstTemplateTemplateParmPack);
TEMPLATE_KIND(UsingTemplate);
+ TEMPLATE_KIND(DeducedTemplate);
#undef TEMPLATE_KIND
}
llvm_unreachable("Unhandled NameKind enum");
diff --git a/clang-tools-extra/clangd/SemanticHighlighting.cpp b/clang-tools-extra/clangd/SemanticHighlighting.cpp
index a366f1331c2d3..e6d16af2495fe 100644
--- a/clang-tools-extra/clangd/SemanticHighlighting.cpp
+++ b/clang-tools-extra/clangd/SemanticHighlighting.cpp
@@ -1120,6 +1120,7 @@ class CollectExtraHighlightings
case TemplateName::SubstTemplateTemplateParm:
case TemplateName::SubstTemplateTemplateParmPack:
case TemplateName::UsingTemplate:
+ case TemplateName::DeducedTemplate:
// Names that could be resolved to a TemplateDecl are handled elsewhere.
break;
}
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index 8bce4812f0d48..8818314de9364 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -262,6 +262,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
mutable llvm::ContextualFoldingSet<SubstTemplateTemplateParmPackStorage,
ASTContext&>
SubstTemplateTemplateParmPacks;
+ mutable llvm::ContextualFoldingSet<DeducedTemplateStorage, ASTContext &>
+ DeducedTemplates;
mutable llvm::ContextualFoldingSet<ArrayParameterType, ASTContext &>
ArrayParameterTypes;
@@ -2247,6 +2249,9 @@ class ASTContext : public RefCountedBase<ASTContext> {
unsigned Index,
bool Final) const;
+ TemplateName getDeducedTemplateName(TemplateName Underlying,
+ DefaultArguments DefaultArgs) const;
+
enum GetBuiltinTypeError {
/// No error
GE_None,
@@ -2726,7 +2731,8 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// template name uses the shortest form of the dependent
/// nested-name-specifier, which itself contains all canonical
/// types, values, and templates.
- TemplateName getCanonicalTemplateName(const TemplateName &Name) const;
+ TemplateName getCanonicalTemplateName(TemplateName Name,
+ bool IgnoreDeduced = false) const;
/// Determine whether the given template names refer to the same
/// template.
diff --git a/clang/include/clang/AST/ASTImporter.h b/clang/include/clang/AST/ASTImporter.h
index 4ffd913846575..7b890bdf492fa 100644
--- a/clang/include/clang/AST/ASTImporter.h
+++ b/clang/include/clang/AST/ASTImporter.h
@@ -485,6 +485,11 @@ class TypeSourceInfo;
/// the declarations it contains.
[[nodiscard]] llvm::Error ImportDefinition(Decl *From);
+ llvm::Error
+ ImportTemplateArguments(ArrayRef<TemplateArgument> FromArgs,
+ SmallVectorImpl<TemplateArgument> &ToArgs);
+ Expected<TemplateArgument> Import(const TemplateArgument &From);
+
/// Cope with a name conflict when importing a declaration into the
/// given context.
///
diff --git a/clang/include/clang/AST/DependenceFlags.h b/clang/include/clang/AST/DependenceFlags.h
index 3b3c1afb096ad..bdcaabc143cc4 100644
--- a/clang/include/clang/AST/DependenceFlags.h
+++ b/clang/include/clang/AST/DependenceFlags.h
@@ -315,6 +315,11 @@ toTemplateNameDependence(NestedNameSpecifierDependence D) {
return Dependence(D).templateName();
}
+inline TemplateNameDependence
+toTemplateNameDependence(TemplateArgumentDependence D) {
+ return Dependence(D).templateName();
+}
+
LLVM_ENABLE_BITMASK_ENUMS_IN_NAMESPACE();
} // namespace clang
diff --git a/clang/include/clang/AST/PropertiesBase.td b/clang/include/clang/AST/PropertiesBase.td
index 6df1d93a7ba2e..bd0b316a4958a 100644
--- a/clang/include/clang/AST/PropertiesBase.td
+++ b/clang/include/clang/AST/PropertiesBase.td
@@ -750,6 +750,23 @@ let Class = PropertyTypeCase<TemplateName, "SubstTemplateTemplateParmPack"> in {
return ctx.getSubstTemplateTemplateParmPack(argumentPack, associatedDecl, index, final);
}]>;
}
+let Class = PropertyTypeCase<TemplateName, "DeducedTemplate"> in {
+ def : ReadHelper<[{
+ auto DTS = node.getAsDeducedTemplateName();
+ }]>;
+ def : Property<"underlying", TemplateName> {
+ let Read = [{ DTS->getUnderlying() }];
+ }
+ def : Property<"startPos", UInt32> {
+ let Read = [{ DTS->getDefaultArguments().StartPos }];
+ }
+ def : Property<"defaultArgs", Array<TemplateArgument>> {
+ let Read = [{ DTS->getDefaultArguments().Args }];
+ }
+ def : Creator<[{
+ return ctx.getDeducedTemplateName(underlying, {startPos, defaultArgs});
+ }]>;
+}
// Type cases for TemplateArgument.
def : PropertyTypeKind<TemplateArgument, TemplateArgumentKind,
diff --git a/clang/include/clang/AST/TemplateName.h b/clang/include/clang/AST/TemplateName.h
index 988a55acd2252..bd70954a42e0d 100644
--- a/clang/include/clang/AST/TemplateName.h
+++ b/clang/include/clang/AST/TemplateName.h
@@ -34,6 +34,7 @@ class NestedNameSpecifier;
enum OverloadedOperatorKind : int;
class OverloadedTemplateStorage;
class AssumedTemplateStorage;
+class DeducedTemplateStorage;
struct PrintingPolicy;
class QualifiedTemplateName;
class SubstTemplateTemplateParmPackStorage;
@@ -50,16 +51,17 @@ class UncommonTemplateNameStorage {
enum Kind {
Overloaded,
Assumed, // defined in DeclarationName.h
+ Deduced,
SubstTemplateTemplateParm,
SubstTemplateTemplateParmPack
};
struct BitsTag {
LLVM_PREFERRED_TYPE(Kind)
- unsigned Kind : 2;
+ unsigned Kind : 3;
// The template parameter index.
- unsigned Index : 15;
+ unsigned Index : 14;
/// The pack index, or the number of stored templates
/// or template arguments, depending on which subclass we have.
@@ -90,6 +92,12 @@ class UncommonTemplateNameStorage {
: nullptr;
}
+ DeducedTemplateStorage *getAsDeducedTemplateName() {
+ return Bits.Kind == Deduced
+ ? reinterpret_cast<DeducedTemplateStorage *>(this)
+ : nullptr;
+ }
+
SubstTemplateTemplateParmStorage *getAsSubstTemplateTemplateParm() {
return Bits.Kind == SubstTemplateTemplateParm
? reinterpret_cast<SubstTemplateTemplateParmStorage *>(this)
@@ -172,6 +180,13 @@ class SubstTemplateTemplateParmPackStorage : public UncommonTemplateNameStorage,
unsigned Index, bool Final);
};
+struct DefaultArguments {
+ unsigned StartPos;
+ ArrayRef<TemplateArgument> Args;
+
+ operator bool() const { return !Args.empty(); }
+};
+
/// Represents a C++ template name within the type system.
///
/// A C++ template name refers to a template within the C++ type
@@ -245,6 +260,10 @@ class TemplateName {
/// A template name that refers to a template declaration found through a
/// specific using shadow declaration.
UsingTemplate,
+
+ /// A template name that refers to another TemplateName with deduced default
+ /// arguments.
+ DeducedTemplate,
};
TemplateName() = default;
@@ -256,6 +275,7 @@ class TemplateName {
explicit TemplateName(QualifiedTemplateName *Qual);
explicit TemplateName(DependentTemplateName *Dep);
explicit TemplateName(UsingShadowDecl *Using);
+ explicit TemplateName(DeducedTemplateStorage *Deduced);
/// Determine whether this template name is NULL.
bool isNull() const;
@@ -272,6 +292,12 @@ class TemplateName {
/// set of function templates, returns NULL.
TemplateDecl *getAsTemplateDecl() const;
+ /// Retrieves the underlying template declaration that
+ /// this template name refers to, along with the
+ /// deduced default arguments, if any.
+ std::pair<TemplateDecl *, DefaultArguments>
+ getTemplateDeclAndDefaultArgs() const;
+
/// Retrieve the underlying, overloaded function template
/// declarations that this template name refers to, if known.
///
@@ -312,6 +338,11 @@ class TemplateName {
/// template declaration is introduced, if any.
UsingShadowDecl *getAsUsingShadowDecl() const;
+ /// Retrieve the deduced template info, if any.
+ DeducedTemplateStorage *getAsDeducedTemplateName() const;
+
+ std::optional<TemplateName> desugar(bool IgnoreDeduced) const;
+
TemplateName getUnderlying() const;
TemplateNameDependence getDependence() const;
@@ -409,6 +440,30 @@ class SubstTemplateTemplateParmStorage
std::optional<unsigned> PackIndex);
};
+class DeducedTemplateStorage : public UncommonTemplateNameStorage,
+ public llvm::FoldingSetNode {
+ friend class ASTContext;
+
+ TemplateName Underlying;
+
+ DeducedTemplateStorage(TemplateName Underlying,
+ const DefaultArguments &DefArgs);
+
+public:
+ TemplateName getUnderlying() const { return Underlying; }
+
+ DefaultArguments getDefaultArguments() const {
+ return {/*StartPos=*/Bits.Index,
+ /*Args=*/{reinterpret_cast<const TemplateArgument *>(this + 1),
+ Bits.Data}};
+ }
+
+ void Profile(llvm::FoldingSetNodeID &ID, ASTContext &Context);
+
+ static void Profile(llvm::FoldingSetNodeID &ID, ASTContext &Context,
+ TemplateName Underlying, const DefaultArguments &DefArgs);
+};
+
inline TemplateName TemplateName::getUnderlying() const {
if (SubstTemplateTemplateParmStorage *subst
= getAsSubstTemplateTemplateParm())
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 4d4579fcfd456..ce7066a76eaec 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -9210,6 +9210,9 @@ class Sema final : public SemaBase {
/// receive true if the cause for the error is the associated constraints of
/// the template not being satisfied by the template arguments.
///
+ /// \param DefaultArgs any default arguments from template specialization
+ /// deduction.
+ ///
/// \param PartialOrderingTTP If true, assume these template arguments are
/// the injected template arguments for a template template parameter.
/// This will relax the requirement that all its possible uses are valid:
@@ -9219,7 +9222,8 @@ class Sema final : public SemaBase {
/// \returns true if an error occurred, false otherwise.
bool CheckTemplateArgumentList(
TemplateDecl *Template, SourceLocation TemplateLoc,
- TemplateArgumentListInfo &TemplateArgs, bool PartialTemplateArgs,
+ TemplateArgumentListInfo &TemplateArgs,
+ const DefaultArguments &DefaultArgs, bool PartialTemplateArgs,
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted,
bool UpdateArgsWithConversions = true,
@@ -9718,8 +9722,8 @@ class Sema final : public SemaBase {
sema::TemplateDeductionInfo &Info);
bool isTemplateTemplateParameterAtLeastAsSpecializedAs(
- TemplateParameterList *PParam, TemplateDecl *AArg, SourceLocation Loc,
- bool IsDeduced);
+ TemplateParameterList *PParam, TemplateDecl *AArg,
+ const DefaultArguments &DefaultArgs, SourceLocation Loc, bool IsDeduced);
void MarkUsedTemplateParameters(const Expr *E, bool OnlyDeduced,
unsigned Depth, llvm::SmallBitVector &Used);
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index cd76b8aa271da..9c8f92cd60b7b 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -880,8 +880,8 @@ ASTContext::ASTContext(LangOptions &LOpts, SourceManager &SM,
TemplateSpecializationTypes(this_()),
DependentTemplateSpecializationTypes(this_()), AutoTypes(this_()),
DependentBitIntTypes(this_()), SubstTemplateTemplateParmPacks(this_()),
- ArrayParameterTypes(this_()), CanonTemplateTemplateParms(this_()),
- SourceMgr(SM), LangOpts(LOpts),
+ DeducedTemplates(this_()), ArrayParameterTypes(this_()),
+ CanonTemplateTemplateParms(this_()), SourceMgr(SM), LangOpts(LOpts),
NoSanitizeL(new NoSanitizeList(LangOpts.NoSanitizeFiles, SM)),
XRayFilter(new XRayFunctionFilter(LangOpts.XRayAlwaysInstrumentFiles,
LangOpts.XRayNeverInstrumentFiles,
@@ -5043,7 +5043,12 @@ QualType ASTContext::getCanonicalTemplateSpecializationType(
"No dependent template names here!");
// Build the canonical template specialization type.
- TemplateName CanonTemplate = getCanonicalTemplateName(Template);
+ // Any DeducedTemplateNames are ignored, because the effective name of a TST
+ // accounts for the TST arguments laid over any default arguments contained in
+ // its name.
+ TemplateName CanonTemplate =
+ getCanonicalTemplateName(Template, /*IgnoreDeduced=*/true);
+
bool AnyNonCanonArgs = false;
auto CanonArgs =
::getCanonicalTemplateArguments(*this, Args, AnyNonCanonArgs);
@@ -6327,16 +6332,22 @@ ASTContext::getNameForTemplate(TemplateName Name,
case TemplateName::UsingTemplate:
return DeclarationNameInfo(Name.getAsUsingShadowDecl()->getDeclName(),
NameLoc);
+ case TemplateName::DeducedTemplate: {
+ DeducedTemplateStorage *DTS = Name.getAsDeducedTemplateName();
+ return getNameForTemplate(DTS->getUnderlying(), NameLoc);
+ }
}
llvm_unreachable("bad template name kind!");
}
-TemplateName
-ASTContext::getCanonicalTemplateName(const TemplateName &Name) const {
+TemplateName ASTContext::getCanonicalTemplateName(TemplateName Name,
+ bool IgnoreDeduced) const {
+ while (std::optional<TemplateName> UnderlyingOrNone =
+ Name.desugar(IgnoreDeduced))
+ Name = *UnderlyingOrNone;
+
switch (Name.getKind()) {
- case TemplateName::UsingTemplate:
- case TemplateName::QualifiedTemplate:
case TemplateName::Template: {
TemplateDecl *Template = Name.getAsTemplateDecl();
if (auto *TTP = dyn_cast<TemplateTemplateParmDecl>(Template))
@@ -6356,12 +6367,6 @@ ASTContext::getCanonicalTemplateName(const TemplateName &Name) const {
return DTN->CanonicalTemplateName;
}
- case TemplateName::SubstTemplateTemplateParm: {
- SubstTemplateTemplateParmStorage *subst
- = Name.getAsSubstTemplateTemplateParm();
- return getCanonicalTemplateName(subst->getReplacement());
- }
-
case TemplateName::SubstTemplateTemplateParmPack: {
SubstTemplateTemplateParmPackStorage *subst =
Name.getAsSubstTemplateTemplateParmPack();
@@ -6371,6 +6376,70 @@ ASTContext::getCanonicalTemplateName(const TemplateName &Name) const {
canonArgPack, subst->getAssociatedDecl()->getCanonicalDecl(),
subst->getFinal(), subst->getIndex());
}
+ case TemplateName::DeducedTemplate: {
+ assert(IgnoreDeduced == false);
+ DeducedTemplateStorage *DTS = Name.getAsDeducedTemplateName();
+ DefaultArguments DefArgs = DTS->getDefaultArguments();
+ TemplateName Underlying = DTS->getUnderlying();
+
+ bool NonCanonical = false;
+ TemplateName CanonUnderlying =
+ getCanonicalTemplateName(Underlying, /*IgnoreDeduced=*/true);
+ NonCanonical |= CanonUnderlying != Underlying;
+ auto CanonArgs =
+ getCanonicalTemplateArguments(*this, DefArgs.Args, NonCanonical);
+ {
+ unsigned NumArgs = CanonArgs.size() - 1;
+ auto handleParamDefArg = [&](const TemplateArgument &ParamDefArg,
+ unsigned I) {
+ auto CanonParamDefArg = getCanonicalTemplateArgument(ParamDefArg);
+ TemplateArgument &CanonDefArg = CanonArgs[I];
+ if (CanonDefArg.structurallyEquals(CanonParamDefArg))
+ return;
+ if (I == NumArgs)
+ CanonArgs.pop_back();
+ NonCanonical = true;
+ };
+ auto handleParam = [&](auto *TP, int I) -> bool {
+ if (!TP->hasDefaultArgument())
+ return true;
+ handleParamDefArg(TP->getDefaultArgument().getArgument(), I);
+ return false;
+ };
+
+ ArrayRef<NamedDecl *> Params = CanonUnderlying.getAsTemplateDecl()
+ ->getTemplateParameters()
+ ->asArray();
+ assert(CanonArgs.size() <= Params.size());
+ for (int I = NumArgs; I >= 0; --I) {
+ switch (auto *Param = Params[I]; Param->getKind()) {
+ case NamedDecl::TemplateTypeParm:
+ if (handleParam(cast<TemplateTypeParmDecl>(Param), I))
+ break;
+ continue;
+ case NamedDecl::NonTypeTemplateParm:
+ if (handleParam(cast<NonTypeTemplateParmDecl>(Param), I))
+ break;
+ continue;
+ case NamedDecl::TemplateTemplateParm:
+ if (handleParam(cast<TemplateTemplateParmDecl>(Param), I))
+ break;
+ continue;
+ default:
+ llvm_unreachable("Unexpected template parameter kind");
+ }
+ break;
+ }
+ }
+ return NonCanonical ? getDeducedTemplateName(
+ CanonUnderlying,
+ /*DefaultArgs=*/{DefArgs.StartPos, CanonArgs})
+ : Name;
+ }
+ case TemplateName::UsingTemplate:
+ case TemplateName::QualifiedTemplate:
+ case TemplateName::SubstTemplateTemplateParm:
+ llvm_unreachable("always sugar node");
}
llvm_unreachable("bad template name!");
@@ -6868,7 +6937,7 @@ ASTContext::getCanonicalTemplateArgument(const TemplateArgument &Arg) const {
case TemplateArgument::StructuralValue:
return TemplateArgument(*this,
getCanonicalType(Arg.getStructuralValueType()),
- Arg.getAsStructuralValue());
+ Arg.getAsStructuralValue(), Arg.getIsDefaulted());
case TemplateArgument::Type:
return TemplateArgument(getCanonicalType(Arg.getAsType()),
@@ -6880,8 +6949,10 @@ ASTContext::getCanonicalTemplateArgument(const TemplateArgument &Arg) const {
*this, Arg.pack_elements(), AnyNonCanonArgs);
if (!AnyNonCanonArgs)
return Arg;
- return TemplateArgument::CreatePackCopy(const_cast<ASTContext &>(*this),
- CanonArgs);
+ auto NewArg = TemplateArgument::CreatePackCopy(
+ const_cast<ASTContext &>(*this), CanonArgs);
+ NewArg.setIsDefaulted(Arg.getIsDefaulted());
+ return NewArg;
}
}
@@ -9434,6 +9505,32 @@ ASTContext::getSubstTemplateTemplateParmPack(const TemplateArgument &ArgPack,
return TemplateName(Subst);
}
+/// Retrieve the template name that represents a template name
+/// deduced from a specialization.
+TemplateName
+ASTContext::getDeducedTemplateName(TemplateName Underlying,
+ DefaultArguments DefaultArgs) const {
+ if (!DefaultArgs)
+ return Underlying;
+
+ auto &Self = const_cast<ASTContext &>(*this);
+ llvm::FoldingSetNodeID ID;
+ DeducedTemplateStorage::Profile(ID, Self, Underlying, DefaultArgs);
+
+ void *InsertPos = nullptr;
+ DeducedTemplateStorage *DTS =
+ DeducedTemplates.FindNodeOrInsertPos(ID, InsertPos);
+ if (!DTS) {
+ void *Mem = Allocate(sizeof(DeducedTemplateStorage) +
+ sizeof(TemplateArgument) * DefaultArgs.Args.size(),
+ alignof(DeducedTemplateStorage));
+ DTS = new (Mem) DeducedTemplateStorage(Underlying, DefaultArgs);
+ DeducedTemplates.InsertNode(DTS, InsertPos);
+ }
+
+ return TemplateName(DTS);
+}
+
/// getFromTargetType - Given one of the integer types provided by
/// TargetInfo, produce the corresponding type. The unsigned @p Type
/// is actually a value of type @c TargetInfo::IntType.
diff --git a/clang/lib/AST/ASTDiagnostic.cpp b/clang/lib/AST/ASTDiagnostic.cpp
index 0680ff5e3a385..18463d72514b1 100644
--- a/clang/lib/AST/ASTDiagnostic.cpp
+++ b/clang/lib/AST/ASTDiagnostic.cpp
@@ -1114,8 +1114,8 @@ class TemplateDiff {
// These functions build up the template diff tree, including functions to
// retrieve and compare temp...
[truncated]
|
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
4 times, most recently
from
6879178
to
d12c7d5
Compare
cor3ntin
reviewed
Jun 12, 2024
| @@ -9219,7 +9222,8 @@ class Sema final : public SemaBase { | |||
| /// \returns true if an error occurred, false otherwise. | |||
| bool CheckTemplateArgumentList( | |||
| TemplateDecl *Template, SourceLocation TemplateLoc, | |||
| TemplateArgumentListInfo &TemplateArgs, bool PartialTemplateArgs, | |||
| TemplateArgumentListInfo &TemplateArgs, | |||
There was a problem hiding this comment.
I wonder if instead we want to add an additional method.
In a lot of places through the rest of the patch, DefaultArgs is defaulted
There was a problem hiding this comment.
I am not seeing a worthwhile tradeoff.
Just the function signature is hugely complicated, and would need to be duplicated.
The implementation would be mostly the same, with a small block which would be omitted in one of the implementations.
What did you have in mind?
There was a problem hiding this comment.
bool CheckTemplateArgumentList(
TemplateDecl *Template, SourceLocation TemplateLoc,
TemplateArgumentListInfo &TemplateArgs,
bool PartialTemplateArgs,
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted,
bool UpdateArgsWithConversions = true,
bool *ConstraintsNotSatisfied = nullptr, bool PartialOrderingTTP = false, const DefaultArguments * DefaultArgs = nullptr);
bool CheckTemplateArgumentList(
TemplateDecl *Template, SourceLocation TemplateLoc,
TemplateArgumentListInfo &TemplateArgs,
const DefaultArguments & DefaultArgs,
SmallVectorImpl<TemplateArgument> &SugaredConverted,
SmallVectorImpl<TemplateArgument> &CanonicalConverted) {
return CheckTemplateArgumentList(Template, TemplateLoc,
/*PartialTemplateArgs=*/false,
SugaredConverted, CanonicalConverted,
/*UpdateArgsWithConversions=*/true,
nullptr, false, &DefaultArgs);
}Maybe something like that?
There was a problem hiding this comment.
Yeah. One issue I have often had with these functions with large amount of both defaulted and non-defaulted parameters, is that you would want to extend it by changing the signature, then arguments would match parameters incorrectly, but this would not cause a hard error on all of the call sites.
I could have easily added DefaultArgs as defaulted empty here, but chose not to due to this reason.
Besides that, overloading functions with such huge numbers of parameters creates some confusion as well.
I'd slightly prefer if we avoided that, but don't have strong enough feelings to go on a crusade against it.
There was a problem hiding this comment.
Can we think about ways to simplify these interface in a subsequent patch?
@AaronBallman @erichkeane for additional opinions
There was a problem hiding this comment.
There's enough repeated stuff though these interfaces here, I don't think I'd mind a followup that created an object to contain all the related stuff. For example, the Sugared/Canonical vectors shoudl probably be a vectro of TempalteArgument pairs (or more likely their own structure) or something? But I'm open to other ideas as well (agian in a followup).
There was a problem hiding this comment.
The problem with the separate Sugared / Converted lists I already have an incomplete patch to address that, though I am currently working on something else.
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
from
d12c7d5
to
f05e859
Compare
|
FYI itanium-cxx-abi/cxx-abi#184 is the tracking issue for the mangling rules we need here. |
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
from
f05e859
to
b238961
Compare
Endilll
reviewed
Jun 19, 2024
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
from
b238961
to
63d51f7
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
from
63d51f7
to
9d06fd6
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-fix-GH18291
branch
from
89e5db4
to
3771ffa
Compare
mizvekov
force-pushed
the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
from
3f95419
to
2729c98
Compare
erichkeane
reviewed
Sep 4, 2024
…cwg2398-ttp-matches-class-template
cor3ntin
reviewed
Sep 5, 2024
Comment on lines
+649
to
+650
| // FIXME: We can't reach here. | ||
| llvm_unreachable("unimplemented"); |
There was a problem hiding this comment.
is it unreachable or just unimplemented?
If deduced arguments are only used during partial ordering they should never need to be compared for structural equivalence... maybe. I am not actually sure, but the fix me and the strings should agree with each other!
There was a problem hiding this comment.
Well it's both, ideally we should implement this, but we don't implement other stuff that would even allow us to reach here.
There was a problem hiding this comment.
Hum, fair enough. It's hard to me to understand how we could ever get there though
| @@ -9219,7 +9222,8 @@ class Sema final : public SemaBase { | |||
| /// \returns true if an error occurred, false otherwise. | |||
| bool CheckTemplateArgumentList( | |||
| TemplateDecl *Template, SourceLocation TemplateLoc, | |||
| TemplateArgumentListInfo &TemplateArgs, bool PartialTemplateArgs, | |||
| TemplateArgumentListInfo &TemplateArgs, | |||
There was a problem hiding this comment.
Can we think about ways to simplify these interface in a subsequent patch?
@AaronBallman @erichkeane for additional opinions
Co-authored-by: cor3ntin <corentinjabot@gmail.com>
Co-authored-by: cor3ntin <corentinjabot@gmail.com>
Co-authored-by: cor3ntin <corentinjabot@gmail.com>
|
BTW
Are there cases where we crash because of missing mangling / mangle incorrectly? |
This doesn't apply directly to this PR anymore. Since this default argument deduction is restricted to partial ordering in this patch, it shall never appear in mangling. |
…cwg2398-ttp-matches-class-template
cor3ntin
approved these changes
Sep 7, 2024
Comment on lines
+6770
to
+6787
| static const TemplateArgument * | ||
| getDefaultTemplateArgumentOrNone(const NamedDecl *P) { | ||
| auto handleParam = [](auto *TP) -> const TemplateArgument * { | ||
| if (!TP->hasDefaultArgument()) | ||
| return nullptr; | ||
| return &TP->getDefaultArgument().getArgument(); | ||
| }; | ||
| switch (P->getKind()) { | ||
| case NamedDecl::TemplateTypeParm: | ||
| return handleParam(cast<TemplateTypeParmDecl>(P)); | ||
| case NamedDecl::NonTypeTemplateParm: | ||
| return handleParam(cast<NonTypeTemplateParmDecl>(P)); | ||
| case NamedDecl::TemplateTemplateParm: | ||
| return handleParam(cast<TemplateTemplateParmDecl>(P)); | ||
| default: | ||
| llvm_unreachable("Unexpected template parameter kind"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not in this PR, but i wonder if we should have a base class for all these types
There was a problem hiding this comment.
We'd have to break TypeDecl, DeclaratorDecl, TemplateDecl, etc into mixins.
Comment on lines
+649
to
+650
| // FIXME: We can't reach here. | ||
| llvm_unreachable("unimplemented"); |
There was a problem hiding this comment.
Hum, fair enough. It's hard to me to understand how we could ever get there though
Co-authored-by: cor3ntin <corentinjabot@gmail.com>
mizvekov
deleted the
users/mizvekov/clang-cwg2398-ttp-matches-class-template
branch
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
…llvm#94981) This extends default argument deduction to cover class templates as well, applying only to partial ordering, adding to the provisional wording introduced in llvm#89807. This solves some ambuguity introduced in P0522 regarding how template template parameters are partially ordered, and should reduce the negative impact of enabling `-frelaxed-template-template-args` by default. Given the following example: ```C++ template <class T1, class T2 = float> struct A; template <class T3> struct B; template <template <class T4> class TT1, class T5> struct B<TT1<T5>>; // llvm#1 template <class T6, class T7> struct B<A<T6, T7>>; // llvm#2 template struct B<A<int>>; ``` Prior to P0522, `llvm#2` was picked. Afterwards, this became ambiguous. This patch restores the pre-P0522 behavior, `llvm#2` is picked again.
|
Hi @mizvekov, we started seeing crashes after this commit. I'm working on a shareable test case, but here's the assertion failure and the stack trace: |
|
Here's a reduced test case: https://gcc.godbolt.org/z/jxer3W39W As usual, it's hard to say if the code got invalid during automatic reduction, but it at least compiles well with clang before this commit. |
|
Thanks for the repro! I am taking a look. |
mizvekov
added a commit
that referenced
this pull request
Sep 13, 2024
Fixes regression introduced in #94981, reported on the pull-request. Since this fixes a commit which was never released, there are no release notes.
|
Thanks! The fix resolves the crash, but the original code still doesn't compile. See #108491 (comment) |
mizvekov
added a commit
that referenced
this pull request
Sep 16, 2024
Fixes regression introduced in #94981, reported on the pull-request. Since this fixes a commit which was never released, there are no release notes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This extends default argument deduction to cover class templates as well, applying only to partial ordering, adding to the provisional wording introduced in #89807.
This solves some ambuguity introduced in P0522 regarding how template template parameters are partially ordered, and should reduce the negative impact of enabling
-frelaxed-template-template-argsby default.Given the following example:
Prior to P0522,
#2was picked. Afterwards, this became ambiguous. This patch restores the pre-P0522 behavior,#2is picked again.