-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Clang] Add a builtin that deduplicate types into a pack #106730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
2762d75
241cbf4
01e5a36
f05e54d
0ebda66
7b1d3fb
1f2f3d2
ae0898b
6288f96
1957947
11b2e84
0b5b579
00971e3
61c57af
989fdfb
df0b041
1597f4a
e409d0a
bf72abe
7f0700c
d2bfa3e
8ef79e9
b81d054
b01e1bb
bdf79de
925e81e
028a222
937027f
047847a
82351ee
9fa61e1
43b557f
62e8f95
2c0dd40
e047cc4
4496902
1cf22a2
eae6f58
da7ad41
cc97b95
53248dd
1c0beeb
5457414
e0b40c7
869f9a9
deedc41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2186,20 +2186,24 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase { | |
unsigned PackIndex : 15; | ||
}; | ||
|
||
class SubstTemplateTypeParmPackTypeBitfields { | ||
class SubstPackTypeBitfields { | ||
friend class SubstPackType; | ||
friend class SubstTemplateTypeParmPackType; | ||
|
||
LLVM_PREFERRED_TYPE(TypeBitfields) | ||
unsigned : NumTypeBits; | ||
|
||
// The index of the template parameter this substitution represents. | ||
unsigned Index : 16; | ||
|
||
/// The number of template arguments in \c Arguments, which is | ||
/// expected to be able to hold at least 1024 according to [implimits]. | ||
/// However as this limit is somewhat easy to hit with template | ||
/// metaprogramming we'd prefer to keep it as large as possible. | ||
unsigned NumArgs : 16; | ||
|
||
// The index of the template parameter this substitution represents. | ||
// Only used by SubstTemplateTypeParmPackType. We keep it in the same | ||
// class to avoid dealing with complexities of bitfields that go over | ||
// the size of `unsigned`. | ||
unsigned SubstTemplTypeParmPackIndex : 16; | ||
}; | ||
|
||
class TemplateSpecializationTypeBitfields { | ||
|
@@ -2315,7 +2319,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase { | |
VectorTypeBitfields VectorTypeBits; | ||
TemplateTypeParmTypeBitfields TemplateTypeParmTypeBits; | ||
SubstTemplateTypeParmTypeBitfields SubstTemplateTypeParmTypeBits; | ||
SubstTemplateTypeParmPackTypeBitfields SubstTemplateTypeParmPackTypeBits; | ||
SubstPackTypeBitfields SubstPackTypeBits; | ||
TemplateSpecializationTypeBitfields TemplateSpecializationTypeBits; | ||
DependentTemplateSpecializationTypeBitfields | ||
DependentTemplateSpecializationTypeBits; | ||
|
@@ -6654,6 +6658,56 @@ class SubstTemplateTypeParmType final | |
} | ||
}; | ||
|
||
/// Represents the result of substituting a set of types as a template argument | ||
/// that needs to be expanded later. | ||
/// | ||
/// These types are always dependent and produced depending on the situations: | ||
/// - SubstTemplateTypeParmPack is an expansion that had to be delayed, | ||
/// - SubstBuiltinTemplatePackType is an expansion from a builtin. | ||
class SubstPackType : public Type, public llvm::FoldingSetNode { | ||
friend class ASTContext; | ||
|
||
/// A pointer to the set of template arguments that this | ||
/// parameter pack is instantiated with. | ||
const TemplateArgument *Arguments; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who owns these arguments? Are they part of the containing declaration? How do we know which set are ours? It IS a touch confusing we're using the type-bits instead of an arrayref here, but justified :( (Nothing to do with this sentence, just lamenting...). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think so, at least it's the case for SubstTemplateTypeParmPackType. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, the contract is the same as |
||
|
||
protected: | ||
SubstPackType(TypeClass Derived, QualType Canon, | ||
const TemplateArgument &ArgPack); | ||
|
||
public: | ||
unsigned getNumArgs() const { return SubstPackTypeBits.NumArgs; } | ||
|
||
TemplateArgument getArgumentPack() const; | ||
|
||
void Profile(llvm::FoldingSetNodeID &ID); | ||
static void Profile(llvm::FoldingSetNodeID &ID, | ||
const TemplateArgument &ArgPack); | ||
|
||
static bool classof(const Type *T) { | ||
return T->getTypeClass() == SubstTemplateTypeParmPack || | ||
T->getTypeClass() == SubstBuiltinTemplatePack; | ||
} | ||
}; | ||
|
||
/// Represents the result of substituting a builtin template as a pack. | ||
class SubstBuiltinTemplatePackType : public SubstPackType { | ||
friend class ASTContext; | ||
|
||
SubstBuiltinTemplatePackType(QualType Canon, const TemplateArgument &ArgPack); | ||
ilya-biryukov marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
public: | ||
bool isSugared() const { return false; } | ||
QualType desugar() const { return QualType(this, 0); } | ||
|
||
/// Mark that we reuse the Profile. We do not introduce new fields. | ||
using SubstPackType::Profile; | ||
|
||
static bool classof(const Type *T) { | ||
return T->getTypeClass() == SubstBuiltinTemplatePack; | ||
} | ||
}; | ||
|
||
/// Represents the result of substituting a set of types for a template | ||
/// type parameter pack. | ||
/// | ||
|
@@ -6666,7 +6720,7 @@ class SubstTemplateTypeParmType final | |
/// that pack expansion (e.g., when all template parameters have corresponding | ||
/// arguments), this type will be replaced with the \c SubstTemplateTypeParmType | ||
/// at the current pack substitution index. | ||
class SubstTemplateTypeParmPackType : public Type, public llvm::FoldingSetNode { | ||
class SubstTemplateTypeParmPackType : public SubstPackType { | ||
zyn0217 marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it looks like when you made this change you forgot to remove This was flagged by static analysis b/c it is not being initialized in the constructor. If there is a reason to keep it around a FIXME should be put in place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely need to remove it, thanks for mentioning this. |
||
friend class ASTContext; | ||
|
||
/// A pointer to the set of template arguments that this | ||
|
@@ -6692,21 +6746,17 @@ class SubstTemplateTypeParmPackType : public Type, public llvm::FoldingSetNode { | |
|
||
/// Returns the index of the replaced parameter in the associated declaration. | ||
/// This should match the result of `getReplacedParameter()->getIndex()`. | ||
unsigned getIndex() const { return SubstTemplateTypeParmPackTypeBits.Index; } | ||
unsigned getIndex() const { | ||
return SubstPackTypeBits.SubstTemplTypeParmPackIndex; | ||
} | ||
|
||
// This substitution will be Final, which means the substitution will be fully | ||
// sugared: it doesn't need to be resugared later. | ||
bool getFinal() const; | ||
|
||
unsigned getNumArgs() const { | ||
return SubstTemplateTypeParmPackTypeBits.NumArgs; | ||
} | ||
|
||
bool isSugared() const { return false; } | ||
QualType desugar() const { return QualType(this, 0); } | ||
|
||
TemplateArgument getArgumentPack() const; | ||
|
||
void Profile(llvm::FoldingSetNodeID &ID); | ||
static void Profile(llvm::FoldingSetNodeID &ID, const Decl *AssociatedDecl, | ||
unsigned Index, bool Final, | ||
|
@@ -6936,9 +6986,7 @@ class TemplateSpecializationType : public Type, public llvm::FoldingSetNode { | |
TemplateSpecializationTypeBits.NumArgs}; | ||
} | ||
|
||
bool isSugared() const { | ||
return !isDependentType() || isCurrentInstantiation() || isTypeAlias(); | ||
} | ||
bool isSugared() const; | ||
|
||
QualType desugar() const { | ||
return isTypeAlias() ? getAliasedType() : getCanonicalTypeInternal(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -228,7 +228,9 @@ void threadSafetyCleanup(BeforeSet *Cache); | |
|
||
// FIXME: No way to easily map from TemplateTypeParmTypes to | ||
// TemplateTypeParmDecls, so we have this horrible PointerUnion. | ||
typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType *, NamedDecl *>, | ||
typedef std::pair<llvm::PointerUnion<const TemplateTypeParmType *, NamedDecl *, | ||
const TemplateSpecializationType *, | ||
const SubstBuiltinTemplatePackType *>, | ||
Comment on lines
+231
to
+233
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're using the last 2 bits of PointerUnion on 32 bit systems, so it might be much harder if we want to add a new type of pack in future. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we hit this limit, we could either use a less optimized representation or figure out some clever trick to change it. I can see various options to refactor this, but I would like to discuss this separately and focus on other aspects of this PR in this review. Would that work for you? Do I need to leave a TODO: behind or open a separate github issue to track this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could leave it as-is :) No fixme nor issue is required because one would notice that when adding new types |
||
SourceLocation> | ||
UnexpandedParameterPack; | ||
|
||
|
@@ -13487,8 +13489,6 @@ class Sema final : public SemaBase { | |
~ArgPackSubstIndexRAII() { Self.ArgPackSubstIndex = OldSubstIndex; } | ||
}; | ||
|
||
friend class ArgumentPackSubstitutionRAII; | ||
|
||
void pushCodeSynthesisContext(CodeSynthesisContext Ctx); | ||
void popCodeSynthesisContext(); | ||
|
||
|
@@ -14504,7 +14504,8 @@ class Sema final : public SemaBase { | |
bool CheckParameterPacksForExpansion( | ||
SourceLocation EllipsisLoc, SourceRange PatternRange, | ||
ArrayRef<UnexpandedParameterPack> Unexpanded, | ||
const MultiLevelTemplateArgumentList &TemplateArgs, bool &ShouldExpand, | ||
const MultiLevelTemplateArgumentList &TemplateArgs, | ||
bool FailOnPackProducingTemplates, bool &ShouldExpand, | ||
bool &RetainExpansion, UnsignedOrNone &NumExpansions); | ||
|
||
/// Determine the number of arguments in the given pack expansion | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this needed for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe Clangd added it for
SubstBuiltinTemplatePackType
and it seems reasonable as the file uses it directly.I have removed it per your suggestions, but I actually do not see a problem with "include what you see".