[Clang][AST] Store injected template arguments in TemplateParameterList #113579
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Krystian Stasiowski (sdkrystian) ChangesCurrently, we store injected template arguments in
This patch moves the storage and access of injected template arguments from Full diff: https://github.com/llvm/llvm-project/pull/113579.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h
index a4d36f2eacd5d1..07b4e36f3ef05e 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -239,7 +239,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
mutable llvm::ContextualFoldingSet<DependentTemplateSpecializationType,
ASTContext&>
DependentTemplateSpecializationTypes;
- llvm::FoldingSet<PackExpansionType> PackExpansionTypes;
+ mutable llvm::FoldingSet<PackExpansionType> PackExpansionTypes;
mutable llvm::FoldingSet<ObjCObjectTypeImpl> ObjCObjectTypes;
mutable llvm::FoldingSet<ObjCObjectPointerType> ObjCObjectPointerTypes;
mutable llvm::FoldingSet<DependentUnaryTransformType>
@@ -1778,13 +1778,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
ElaboratedTypeKeyword Keyword, NestedNameSpecifier *NNS,
const IdentifierInfo *Name, ArrayRef<TemplateArgument> Args) const;
- TemplateArgument getInjectedTemplateArg(NamedDecl *ParamDecl);
-
- /// Get a template argument list with one argument per template parameter
- /// in a template parameter list, such as for the injected class name of
- /// a class template.
- void getInjectedTemplateArgs(const TemplateParameterList *Params,
- SmallVectorImpl<TemplateArgument> &Args);
+ TemplateArgument getInjectedTemplateArg(NamedDecl *ParamDecl) const;
/// Form a pack expansion type with the given pattern.
/// \param NumExpansions The number of expansions for the pack, if known.
@@ -1795,7 +1789,7 @@ class ASTContext : public RefCountedBase<ASTContext> {
/// if this is the canonical type of another pack expansion type.
QualType getPackExpansionType(QualType Pattern,
std::optional<unsigned> NumExpansions,
- bool ExpectPackInType = true);
+ bool ExpectPackInType = true) const;
QualType getObjCInterfaceType(const ObjCInterfaceDecl *Decl,
ObjCInterfaceDecl *PrevDecl = nullptr) const;
diff --git a/clang/include/clang/AST/DeclTemplate.h b/clang/include/clang/AST/DeclTemplate.h
index 0f0c0bf6e4ef4f..2d2102d3280ba0 100644
--- a/clang/include/clang/AST/DeclTemplate.h
+++ b/clang/include/clang/AST/DeclTemplate.h
@@ -71,6 +71,9 @@ NamedDecl *getAsNamedDecl(TemplateParameter P);
class TemplateParameterList final
: private llvm::TrailingObjects<TemplateParameterList, NamedDecl *,
Expr *> {
+ /// The template argument list of the template parameter list.
+ llvm::PointerUnion<const ASTContext *, TemplateArgument *> InjectedArgs;
+
/// The location of the 'template' keyword.
SourceLocation TemplateLoc;
@@ -196,6 +199,9 @@ class TemplateParameterList final
bool hasAssociatedConstraints() const;
+ /// Get the template argument lisr of the template parameter list.
+ ArrayRef<TemplateArgument> getInjectedTemplateArgs();
+
SourceLocation getTemplateLoc() const { return TemplateLoc; }
SourceLocation getLAngleLoc() const { return LAngleLoc; }
SourceLocation getRAngleLoc() const { return RAngleLoc; }
@@ -793,15 +799,6 @@ class RedeclarableTemplateDecl : public TemplateDecl,
/// The first value in the array is the number of specializations/partial
/// specializations that follow.
GlobalDeclID *LazySpecializations = nullptr;
-
- /// The set of "injected" template arguments used within this
- /// template.
- ///
- /// This pointer refers to the template arguments (there are as
- /// many template arguments as template parameters) for the
- /// template, and is allocated lazily, since most templates do not
- /// require the use of this information.
- TemplateArgument *InjectedArgs = nullptr;
};
/// Pointer to the common data shared by all declarations of this
@@ -927,7 +924,9 @@ class RedeclarableTemplateDecl : public TemplateDecl,
/// Although the C++ standard has no notion of the "injected" template
/// arguments for a template, the notion is convenient when
/// we need to perform substitutions inside the definition of a template.
- ArrayRef<TemplateArgument> getInjectedTemplateArgs();
+ ArrayRef<TemplateArgument> getInjectedTemplateArgs() const {
+ return getTemplateParameters()->getInjectedTemplateArgs();
+ }
using redecl_range = redeclarable_base::redecl_range;
using redecl_iterator = redeclarable_base::redecl_iterator;
@@ -2087,10 +2086,6 @@ class ClassTemplatePartialSpecializationDecl
/// The list of template parameters
TemplateParameterList *TemplateParams = nullptr;
- /// The set of "injected" template arguments used within this
- /// partial specialization.
- TemplateArgument *InjectedArgs = nullptr;
-
/// The class template partial specialization from which this
/// class template partial specialization was instantiated.
///
@@ -2136,9 +2131,10 @@ class ClassTemplatePartialSpecializationDecl
return TemplateParams;
}
- /// Retrieve the template arguments list of the template parameter list
- /// of this template.
- ArrayRef<TemplateArgument> getInjectedTemplateArgs();
+ /// Get the template argument lisr of the template parameter list.
+ ArrayRef<TemplateArgument> getInjectedTemplateArgs() const {
+ return getTemplateParameters()->getInjectedTemplateArgs();
+ }
/// \brief All associated constraints of this partial specialization,
/// including the requires clause and any constraints derived from
@@ -2864,10 +2860,6 @@ class VarTemplatePartialSpecializationDecl
/// The list of template parameters
TemplateParameterList *TemplateParams = nullptr;
- /// The set of "injected" template arguments used within this
- /// partial specialization.
- TemplateArgument *InjectedArgs = nullptr;
-
/// The variable template partial specialization from which this
/// variable template partial specialization was instantiated.
///
@@ -2914,9 +2906,10 @@ class VarTemplatePartialSpecializationDecl
return TemplateParams;
}
- /// Retrieve the template arguments list of the template parameter list
- /// of this template.
- ArrayRef<TemplateArgument> getInjectedTemplateArgs();
+ /// Get the template argument lisr of the template parameter list.
+ ArrayRef<TemplateArgument> getInjectedTemplateArgs() const {
+ return getTemplateParameters()->getInjectedTemplateArgs();
+ }
/// \brief All associated constraints of this partial specialization,
/// including the requires clause and any constraints derived from
diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 4bf8ddd762e9a5..7b602955e8a454 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -5622,7 +5622,7 @@ ASTContext::getDependentTemplateSpecializationType(
return QualType(T, 0);
}
-TemplateArgument ASTContext::getInjectedTemplateArg(NamedDecl *Param) {
+TemplateArgument ASTContext::getInjectedTemplateArg(NamedDecl *Param) const {
TemplateArgument Arg;
if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(Param)) {
QualType ArgType = getTypeDeclType(TTP);
@@ -5666,23 +5666,14 @@ TemplateArgument ASTContext::getInjectedTemplateArg(NamedDecl *Param) {
}
if (Param->isTemplateParameterPack())
- Arg = TemplateArgument::CreatePackCopy(*this, Arg);
+ Arg = TemplateArgument::CreatePackCopy(const_cast<ASTContext &>(*this), Arg);
return Arg;
}
-void
-ASTContext::getInjectedTemplateArgs(const TemplateParameterList *Params,
- SmallVectorImpl<TemplateArgument> &Args) {
- Args.reserve(Args.size() + Params->size());
-
- for (NamedDecl *Param : *Params)
- Args.push_back(getInjectedTemplateArg(Param));
-}
-
QualType ASTContext::getPackExpansionType(QualType Pattern,
std::optional<unsigned> NumExpansions,
- bool ExpectPackInType) {
+ bool ExpectPackInType) const {
assert((!ExpectPackInType || Pattern->containsUnexpandedParameterPack()) &&
"Pack expansions must expand one or more parameter packs");
diff --git a/clang/lib/AST/DeclTemplate.cpp b/clang/lib/AST/DeclTemplate.cpp
index d2d8907b884ec8..34e7bdf6305f7e 100644
--- a/clang/lib/AST/DeclTemplate.cpp
+++ b/clang/lib/AST/DeclTemplate.cpp
@@ -57,7 +57,7 @@ TemplateParameterList::TemplateParameterList(const ASTContext& C,
ArrayRef<NamedDecl *> Params,
SourceLocation RAngleLoc,
Expr *RequiresClause)
- : TemplateLoc(TemplateLoc), LAngleLoc(LAngleLoc), RAngleLoc(RAngleLoc),
+ : InjectedArgs(&C), TemplateLoc(TemplateLoc), LAngleLoc(LAngleLoc), RAngleLoc(RAngleLoc),
NumParams(Params.size()), ContainsUnexpandedParameterPack(false),
HasRequiresClause(RequiresClause != nullptr),
HasConstrainedParameters(false) {
@@ -244,6 +244,17 @@ bool TemplateParameterList::hasAssociatedConstraints() const {
return HasRequiresClause || HasConstrainedParameters;
}
+ArrayRef<TemplateArgument> TemplateParameterList::getInjectedTemplateArgs() {
+ if (const auto *Context = InjectedArgs.dyn_cast<const ASTContext *>()) {
+ TemplateArgument *Args = new (*Context) TemplateArgument[size()];
+ llvm::transform(*this, Args, [&](NamedDecl *ND) {
+ return Context->getInjectedTemplateArg(ND);
+ });
+ InjectedArgs = Args;
+ }
+ return {InjectedArgs.get<TemplateArgument *>(), NumParams};
+}
+
bool TemplateParameterList::shouldIncludeTypeForArgument(
const PrintingPolicy &Policy, const TemplateParameterList *TPL,
unsigned Idx) {
@@ -396,22 +407,6 @@ void RedeclarableTemplateDecl::addSpecializationImpl(
SETraits::getDecl(Entry));
}
-ArrayRef<TemplateArgument> RedeclarableTemplateDecl::getInjectedTemplateArgs() {
- TemplateParameterList *Params = getTemplateParameters();
- auto *CommonPtr = getCommonPtr();
- if (!CommonPtr->InjectedArgs) {
- auto &Context = getASTContext();
- SmallVector<TemplateArgument, 16> TemplateArgs;
- Context.getInjectedTemplateArgs(Params, TemplateArgs);
- CommonPtr->InjectedArgs =
- new (Context) TemplateArgument[TemplateArgs.size()];
- std::copy(TemplateArgs.begin(), TemplateArgs.end(),
- CommonPtr->InjectedArgs);
- }
-
- return llvm::ArrayRef(CommonPtr->InjectedArgs, Params->size());
-}
-
//===----------------------------------------------------------------------===//
// FunctionTemplateDecl Implementation
//===----------------------------------------------------------------------===//
@@ -631,13 +626,10 @@ ClassTemplateDecl::getInjectedClassNameSpecialization() {
// expansion (14.5.3) whose pattern is the name of the template parameter
// pack.
ASTContext &Context = getASTContext();
- TemplateParameterList *Params = getTemplateParameters();
- SmallVector<TemplateArgument, 16> TemplateArgs;
- Context.getInjectedTemplateArgs(Params, TemplateArgs);
TemplateName Name = Context.getQualifiedTemplateName(
/*NNS=*/nullptr, /*TemplateKeyword=*/false, TemplateName(this));
CommonPtr->InjectedClassNameType =
- Context.getTemplateSpecializationType(Name, TemplateArgs);
+ Context.getTemplateSpecializationType(Name, getTemplateParameters()->getInjectedTemplateArgs());
return CommonPtr->InjectedClassNameType;
}
@@ -1185,20 +1177,6 @@ SourceRange ClassTemplatePartialSpecializationDecl::getSourceRange() const {
return Range;
}
-ArrayRef<TemplateArgument>
-ClassTemplatePartialSpecializationDecl::getInjectedTemplateArgs() {
- TemplateParameterList *Params = getTemplateParameters();
- auto *First = cast<ClassTemplatePartialSpecializationDecl>(getFirstDecl());
- if (!First->InjectedArgs) {
- auto &Context = getASTContext();
- SmallVector<TemplateArgument, 16> TemplateArgs;
- Context.getInjectedTemplateArgs(Params, TemplateArgs);
- First->InjectedArgs = new (Context) TemplateArgument[TemplateArgs.size()];
- std::copy(TemplateArgs.begin(), TemplateArgs.end(), First->InjectedArgs);
- }
- return llvm::ArrayRef(First->InjectedArgs, Params->size());
-}
-
//===----------------------------------------------------------------------===//
// FriendTemplateDecl Implementation
//===----------------------------------------------------------------------===//
@@ -1549,20 +1527,6 @@ SourceRange VarTemplatePartialSpecializationDecl::getSourceRange() const {
return Range;
}
-ArrayRef<TemplateArgument>
-VarTemplatePartialSpecializationDecl::getInjectedTemplateArgs() {
- TemplateParameterList *Params = getTemplateParameters();
- auto *First = cast<VarTemplatePartialSpecializationDecl>(getFirstDecl());
- if (!First->InjectedArgs) {
- auto &Context = getASTContext();
- SmallVector<TemplateArgument, 16> TemplateArgs;
- Context.getInjectedTemplateArgs(Params, TemplateArgs);
- First->InjectedArgs = new (Context) TemplateArgument[TemplateArgs.size()];
- std::copy(TemplateArgs.begin(), TemplateArgs.end(), First->InjectedArgs);
- }
- return llvm::ArrayRef(First->InjectedArgs, Params->size());
-}
-
static TemplateParameterList *
createMakeIntegerSeqParameterList(const ASTContext &C, DeclContext *DC) {
// typename T
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index db1d7fa237131a..122add795384a6 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -6372,18 +6372,14 @@ bool Sema::isTemplateTemplateParameterAtLeastAsSpecializedAs(
// - Each function template has a single function parameter whose type is
// a specialization of X with template arguments corresponding to the
// template parameters from the respective function template
- SmallVector<TemplateArgument, 8> AArgs;
- Context.getInjectedTemplateArgs(A, AArgs);
+ SmallVector<TemplateArgument, 8> AArgs(A->getInjectedTemplateArgs());
// Check P's arguments against A's parameter list. This will fill in default
// template arguments as needed. AArgs are already correct by construction.
// We can't just use CheckTemplateIdType because that will expand alias
// templates.
- SmallVector<TemplateArgument, 4> PArgs;
+ SmallVector<TemplateArgument, 4> PArgs(P->getInjectedTemplateArgs());
{
- SFINAETrap Trap(*this);
-
- Context.getInjectedTemplateArgs(P, PArgs);
TemplateArgumentListInfo PArgList(P->getLAngleLoc(),
P->getRAngleLoc());
for (unsigned I = 0, N = P->size(); I != N; ++I) {
@@ -6399,6 +6395,7 @@ bool Sema::isTemplateTemplateParameterAtLeastAsSpecializedAs(
}
PArgs.clear();
+ SFINAETrap Trap(*this);
// C++1z [temp.arg.template]p3:
// If the rewrite produces an invalid type, then P is not at least as
// specialized as A.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
sdkrystian
force-pushed
the
perf/injected-targs
branch
from
69f9da5 to
95c86a7
Compare
clang/lib/AST/DeclTemplate.cpp
Outdated
| SourceLocation TemplateLoc, | ||
| SourceLocation LAngleLoc, | ||
| ArrayRef<NamedDecl *> Params, | ||
| SourceLocation RAngleLoc, | ||
| Expr *RequiresClause) | ||
| : TemplateLoc(TemplateLoc), LAngleLoc(LAngleLoc), RAngleLoc(RAngleLoc), | ||
| NumParams(Params.size()), ContainsUnexpandedParameterPack(false), | ||
| : InjectedArgs(&C), TemplateLoc(TemplateLoc), LAngleLoc(LAngleLoc), |
There was a problem hiding this comment.
This trickery with storing the ASTContext seems a little silly. I would vastly prefer one of the following:
1- We just store the AST Context reference in this type.
2- We make getInjectedTemplateArgs take an ASTContext reference.
3- We take a collection in getInjectedTemplateArgs and just fill it.
4- We DON'T have this function (or do #2), and keep the ASTContext version of this function to do the work.
There was a problem hiding this comment.
This trickery with storing the ASTContext seems a little silly.
@erichkeane Is there any particular reason why? We already do something similar in e.g. Redeclarable::DeclLink. The injected template arguments are lazily allocated & cached, so we don't need the pointer to ASTContext once the injected template arguments have been allocated. Since we can use a PointerUnion to store both values, I think adding yet another pointer to TemplateParameterList would be rather unjustified.
There was a problem hiding this comment.
Honestly, I strongly dislike it there as well. It seems like quite a hacky way of getting our intended behavior here that is jarring/unreadable every time I see it. I also think that there is some level of awkwardness to doing an allocation in a function whose type doesn't really look like it should be able to allocate.
There was a problem hiding this comment.
I also think that there is some level of awkwardness to doing an allocation in a function whose type doesn't really look like it should be able to allocate.
Since it's an accessor for lazily allocated data, it has to allocate if the data hasn't been allocated yet :)
There was a problem hiding this comment.
Yep, i understand that. Currying is always pretty awkward in languages/programs that don't otherwise lazily evaluate :) It seems to me if the callers to this all have access to ASTContext, we could more easily just require it as a caller-passed arg.
There was a problem hiding this comment.
My hope would be that the callers there could pass on their AST Context, I see the uses in this review are either already have an ASTContext or access to Sema. Could you do that (add it as a parm to the getInjectedTemplateArgs, and see how rough it is for the callers to get the ASTContext), and put it somewhere I can take a look? If it ends up getting too rough in some places, I think i could 'disagree and commit' to this approach.
There was a problem hiding this comment.
Could you do that (add it as a parm to the
getInjectedTemplateArgs, and see how rough it is for the callers to get the ASTContext), and put it somewhere I can take a look?
@erichkeane See 3fd8326
This will break tools that use getInjectedTemplateArgs though (e.g. the tool I'm working on [MrDocs] uses getInjectedTemplateArgs).
There was a problem hiding this comment.
Could you do that (add it as a parm to the
getInjectedTemplateArgs, and see how rough it is for the callers to get the ASTContext), and put it somewhere I can take a look?@erichkeane See 3fd8326
This will break tools that use
getInjectedTemplateArgsthough (e.g. the tool I'm working on [MrDocs] usesgetInjectedTemplateArgs).
Thanks for doing that! I'll note that looks way nicer IMO. I'd hope that downstream users would have access to the ASTContext/Sema in a way that they could use that? Is it REALLY troublesome/not really possible, or is it just a code change?
There was a problem hiding this comment.
Is it REALLY troublesome/not really possible, or is it just a code change?
Fixing existing code should be trivial, I just really dislike "getters" with parameters :P Ultimately, the "right thing" here is very subjective... but I'll cherry-pick 3fd8326 onto this branch & merge, absent any objections
There was a problem hiding this comment.
Fixing existing code should be trivial, I just really dislike "getters" with parameters :P
I'm not THRILLED with it either, though we do this withASTContextall over the place in the codebase to mean 'this probably needs to allocate'. But thank you for your patience/flexibility here.
…st (llvm#113579) Currently, we store injected template arguments in `RedeclarableTemplateDecl::CommonBase`. This approach has a couple problems: 1. We can only access the injected template arguments of `RedeclarableTemplateDecl` derived types, but other `Decl` kinds still make use of the injected arguments (e.g. `ClassTemplatePartialSpecializationDecl`, `VarTemplatePartialSpecializationDecl`, and `TemplateTemplateParmDecl`). 2. Accessing the injected template arguments requires the common data structure to be allocated. This may occur before we determine whether a previous declaration exists (e.g. when comparing constraints), so if the template _is_ a redeclaration, we end up discarding the common data structure. This patch moves the storage and access of injected template arguments from `RedeclarableTemplateDecl` to `TemplateParameterList`.
Currently, we store injected template arguments in
RedeclarableTemplateDecl::CommonBase. This approach has a couple problems:RedeclarableTemplateDeclderived types, but otherDeclkinds still make use of the injected arguments (e.g.ClassTemplatePartialSpecializationDecl,VarTemplatePartialSpecializationDecl, andTemplateTemplateParmDecl).This patch moves the storage and access of injected template arguments from
RedeclarableTemplateDecltoTemplateParameterList.