[PATCH] Fix core-DR1755 & 727 & llvm-PR17294 & PR16906 - partial & explicit specializations of member templates (class/variable/function)
Richard Smith
richard at metafoo.co.uk
Fri Jul 25 15:25:54 PDT 2014
Restarted reviewing this, then noticed you've updated it since I last worked on it. Alas. So... you get two different versions of me reviewing the first ~half of this patch :)
================
Comment at: include/clang/AST/DeclTemplate.h:668
@@ -667,3 +667,3 @@
void setMemberSpecialization() {
- assert(getCommonPtr()->InstantiatedFromMember.getPointer() &&
+ assert(isa<CXXRecordDecl>(getDeclContext()) &&
"Only member templates can be member template specializations");
----------------
Maybe `isCXXClassMember()`?
FIXME: How do we get here for something that's not instantiated from a member?
================
Comment at: include/clang/AST/DeclTemplate.h:807-809
@@ -806,4 +806,5 @@
void *InsertPos);
+public:
+
/// Get the underlying function declaration of the template.
----------------
This whitespace change doesn't reflect our common practice, which is to put a blank line before `public:` and no blank line afterwards.
================
Comment at: include/clang/Sema/Sema.h:364-370
@@ -363,1 +363,9 @@
+ // Set of all the constexpr variable templates whose values have
+ // been used - this helps us distinguish those variable template
+ // specializations who had their initializers instantiated because
+ // they were needed for 'auto' deduction vs those that were needed
+ // because their value was used. We can not explicitly specialize
+ // those whose value was actually used - such as the dimension of
+ // an inclass array member etc.
+ llvm::SmallPtrSet<VarTemplateSpecializationDecl *, 8>
----------------
I don't think we can explicitly specialize in either case. If we needed to instantiate the initializer for any reason, then it's too late to explicitly specialize.
================
Comment at: include/clang/Sema/Sema.h:5295-5297
@@ -5284,4 +5294,5 @@
Scope *S, Declarator &D, TypeSourceInfo *DI,
SourceLocation TemplateKWLoc, TemplateParameterList *TemplateParams,
- StorageClass SC, bool IsPartialSpecialization);
+ StorageClass SC, bool IsPartialSpecialization,
+ MultiTemplateParamsArg TemplateParameterLists);
----------------
Do we really need both `TemplateParams` and `TemplateParameterLists`?
================
Comment at: include/clang/Sema/Sema.h:6087-6093
@@ -6070,1 +6086,9 @@
+ // Given a primary template and the supplied template arguments, return in the
+ // form of a tuple:
+ // 1) the best partial specialization
+ // 2) the deduced argument list (can incorporate default arguments not explicitly supplied)
+ // 3) and whether the returned partial specialization is ambiguous.
+ // If the partial specialization decl is null, none was found, use the primary.
+
+ std::tuple<ClassTemplatePartialSpecializationDecl *,
----------------
Doxygenify this:
1) Use `///`, not `//` (here and elsewhere).
2) To apply this doc comment to both following functions, put `//@{` on the line before the first function and `//@}` on the line after the second one.
================
Comment at: include/clang/Sema/Sema.h:6170-6175
@@ -6124,1 +6169,8 @@
+ /// We are substituting template arguments determined as part of
+ /// template argument deduction performed when an explicit function
+ /// template specialization is being declared to check whether it is
+ /// well-formed and corresponds to a primary function template.
+ /// The Entity is a FunctionTemplateDecl.
+ SubstitutionWhileCheckingDeclarationOfAnExplicitFunctionSpecialization,
+
----------------
Is this any different from `DeducedTemplateArgumentSubstitution`?
================
Comment at: include/clang/Sema/Sema.h:371-372
@@ +370,4 @@
+ // an inclass array member etc.
+ llvm::SmallPtrSet<VarTemplateSpecializationDecl *, 8>
+ ConstexprVarTemplateSpecsValueUsed;
+
----------------
Can you use the `Used` flag for this instead?
FIXME: Is this missing serialization?
================
Comment at: include/clang/Sema/Sema.h:6086
@@ -6069,1 +6085,3 @@
+ // Given a primary template and the supplied template arguments, return in the
+ // form of a tuple:
----------------
Please use `///` comments here and below.
================
Comment at: include/clang/Sema/Sema.h:6088-6090
@@ +6087,5 @@
+ // form of a tuple:
+ // 1) the best partial specialization
+ // 2) the deduced argument list (can incorporate default arguments not explicitly supplied)
+ // 3) and whether the returned partial specialization is ambiguous.
+ // If the partial specialization decl is null, none was found, use the primary.
----------------
Seems complex enough that a `template<typename Decl> struct MostSpecializedPartialSpecialization` would help.
================
Comment at: include/clang/Sema/Sema.h:6108-6109
@@ +6107,4 @@
+ // declaration - including all outer scopes if defined inline.
+ SmallVector<TemplateParameterList *, 4>
+ getTemplateParameterListsOfDeclaration(const Decl *D);
+
----------------
It's generally preferable and conventional to pass in a `SmallVectorImpl` by reference rather than returning a `SmallVector` by value, so the caller gets to choose the `SmallVector` size.
================
Comment at: include/clang/Sema/Sema.h:6174
@@ +6173,3 @@
+ /// The Entity is a FunctionTemplateDecl.
+ SubstitutionWhileCheckingDeclarationOfAnExplicitFunctionSpecialization,
+
----------------
Wow, that's rather long. Maybe just `ExplicitFunctionSpecializationSubstitution`?
================
Comment at: lib/AST/ASTContext.cpp:7933-7949
@@ -7932,2 +7932,19 @@
return false;
+ // If we have a specialization of a variable member template (within a class
+ // template specialization) that does not have an initializer and it was
+ // "instantiated from" an explicitly provided full specialization within a
+ // class member template - and that proto full specialization does have an
+ // initializer, that means that this specialization was simply generated
+ // during consideration of whether that explicitly provided full
+ // specialization is the best specialization to use for a certain template-id,
+ // but was not eventually used - so we do not need to emit it.
+ // For e.g.
+ // template<class T> struct A {
+ // template<class U> static int B;
+ // };
+ // template<class T> template<class U>
+ // int A<T>::B{};
+ // template<class T> template<>
+ // A<T> A<T>::B<int>{};
+ // int x = A<char>::B<float>;
----------------
This should be covered by `isThisDeclarationADefinition`, shouldn't it? Maybe the problem is in that function instead? This use of `A<char>::B<float>` should leave `A<char>::B<int>` as being only a declaration.
================
Comment at: lib/AST/Decl.cpp:1767-1781
@@ +1766,17 @@
+ // constexpr int A<short>::VarT; # Declaration (sans Init)
+ const bool IsExplicitlySpecializedMemberOfImplicitlyInstantiatedClassSpec =
+ ([](const VarDecl *Var) {
+ const ClassTemplateSpecializationDecl *ParentClassSpec =
+ dyn_cast<ClassTemplateSpecializationDecl>(Var->getDeclContext());
+ if (!ParentClassSpec ||
+ ParentClassSpec->getTemplateSpecializationKind() !=
+ TSK_ImplicitInstantiation)
+ return false;
+ assert(!ParentClassSpec->isDependentContext() &&
+ "An implicitly instantiated class should never be dependent");
+
+ return Var->isOutOfLine() && !Var->getFirstDecl()->isOutOfLine() &&
+ Var->getTemplateSpecializationKind() ==
+ TSK_ExplicitSpecialization;
+ }(this));
+
----------------
Please factor this out into a static helper function.
================
Comment at: lib/AST/Decl.cpp:1752-1753
@@ +1751,4 @@
+ const bool HasInit = hasInit();
+ const bool IsFirstDeclOutOfLine = getFirstDecl()->isOutOfLine();
+ const bool IsFirstDeclInClass = !IsFirstDeclOutOfLine;
+
----------------
I'd suggest dropping one of these. Probably the second one.
================
Comment at: lib/AST/Decl.cpp:1764
@@ +1763,3 @@
+ // constexpr int A<short>::VarT; # Declaration (sans Init)
+ const bool IsExplicitlySpecializedMemberOfImplicitlyInstantiatedClassSpec =
+ ([](const VarDecl *Var) {
----------------
Does it really matter whether the surrounding class template specialization is implicitly or explicitly instantiated? I would think you just want to know whether it `isTemplateInstantiation`.
Also, is this just `isMemberSpecialization()`?
================
Comment at: lib/AST/Decl.cpp:1765
@@ +1764,3 @@
+ const bool IsExplicitlySpecializedMemberOfImplicitlyInstantiatedClassSpec =
+ ([](const VarDecl *Var) {
+ const ClassTemplateSpecializationDecl *ParentClassSpec =
----------------
I think MSVC version <old> requires an explicit return type here? Also, this style is not common in Clang and LLVM; we'd usually just use
bool IsVeryLongNameBlahBlah;
{
// Compute value and initialize.
}
================
Comment at: lib/AST/Decl.cpp:1766
@@ +1765,3 @@
+ ([](const VarDecl *Var) {
+ const ClassTemplateSpecializationDecl *ParentClassSpec =
+ dyn_cast<ClassTemplateSpecializationDecl>(Var->getDeclContext());
----------------
Please use `auto` when initializing a variable from the result of `cast` or `dyn_cast`.
================
Comment at: lib/AST/Decl.cpp:1775-1777
@@ +1774,5 @@
+
+ return Var->isOutOfLine() && !Var->getFirstDecl()->isOutOfLine() &&
+ Var->getTemplateSpecializationKind() ==
+ TSK_ExplicitSpecialization;
+ }(this));
----------------
Please add a default reference capture, and use the variables you already have for the first two things here.
================
Comment at: lib/AST/Decl.cpp:1783-1797
@@ +1782,17 @@
+ getTemplateSpecializationKind() == TSK_ExplicitSpecialization;
+ // If we are declaring a partial or full specialization out of line
+ // - and if there already exists a previous in-class declaration,
+ // then this declaration is automatically a definition.
+ // - otherwise it is only a definition if it includes an initializer.
+ // template<class T1, class T2> struct A {
+ // template<class U> static const A<U, T1> var;
+ // template<class U> static const char var<U*>;
+ // };
+ // template<class T1, class T2> template<class U>
+ // const char A<T1, T2>::var<U*>; <-- this is automatically a definition
+ // template<class T1, class T2> template<class U>
+ // const int A<T1, T2>::var<U**>; <-- this is a declaration.
+ // template<class T1, class T2> template<class U>
+ // const int A<T1, T2>::var<U**>{}; <-- this is a definition since it
+ // has an initializer.
+
----------------
That's a really weird rule. Maybe we should take this one back to core. It'd be much simpler to apply the current "definition if it has an initializer" rule in all out-of-line cases.
================
Comment at: lib/AST/Decl.cpp:1803
@@ +1802,3 @@
+
+ if (IsThisDeclOutOfClass) {
+ // If we have an initializer that is out of class, this must be a
----------------
An early bailout if this is `false` would be better.
================
Comment at: lib/AST/Decl.cpp:1806-1807
@@ +1805,4 @@
+ // definition.
+ if (HasInit)
+ return Definition;
+ if (IsPartialOrFullVarTemplateSpecWithPreviousInClassDeclaration)
----------------
Likewise for this.
================
Comment at: lib/AST/Decl.cpp:1808-1816
@@ -1758,7 +1807,11 @@
+ return Definition;
+ if (IsPartialOrFullVarTemplateSpecWithPreviousInClassDeclaration)
+ return Definition;
+ if (IsPartialOrFullVarTemplateSpecialization ||
+ IsExplicitlySpecializedMemberOfImplicitlyInstantiatedClassSpec)
+ return DeclarationOnly;
+ if (IsFirstDeclOutOfLine
? getTemplateSpecializationKind() == TSK_Undeclared
- : getTemplateSpecializationKind() !=
- TSK_ExplicitSpecialization) ||
- isa<VarTemplatePartialSpecializationDecl>(this)))
- return Definition;
- else
- return DeclarationOnly;
+ : getTemplateSpecializationKind() != TSK_ExplicitSpecialization)
+ return Definition;
+ }
----------------
I think this should simplify somewhat. Looks like the general rule is: if the variable is templated in any way, then it's a definition if the first declaration was within a class, otherwise it's a definition.
================
Comment at: lib/AST/DeclTemplate.cpp:1053-1054
@@ -1049,3 +1052,4 @@
P != PEnd; ++P) {
- if (P->getInstantiatedFromMember()->getCanonicalDecl() == DCanon)
- return P->getMostRecentDecl();
+ if (P->getInstantiatedFromMember())
+ if (P->getInstantiatedFromMember()->getCanonicalDecl() == DCanon)
+ return P->getMostRecentDecl();
----------------
Please use
if (auto *D = P->getInstantiatedFromMember())
if (D->getCanonicalDecl() == DCanon)
(both here and above).
================
Comment at: lib/AST/TemplateBase.cpp:24
@@ -23,2 +23,3 @@
#include "clang/Basic/Diagnostic.h"
+#include "clang/Sema/SemaInternal.h"
#include "llvm/ADT/FoldingSet.h"
----------------
Please do not include Sema into AST.
================
Comment at: lib/Sema/SemaDecl.cpp:2853
@@ +2852,3 @@
+ IsExplicitlySpecializedPrimaryVarTemplOfImplicitlyInstantiatedClassSpec =
+ ([](VarDecl *New, VarDecl *Old) {
+ if (VarTemplateDecl *NewVT = New->getDescribedVarTemplate()) {
----------------
As before, I think the lambda hurts readability here.
================
Comment at: lib/Sema/SemaDecl.cpp:2863
@@ +2862,3 @@
+ // need to be redecl connected.
+ assert(New->getDeclContext() == Old->getDeclContext());
+ if (auto *ClassSpec = dyn_cast<ClassTemplateSpecializationDecl>(New->getDeclContext()))
----------------
This assertion can fail, for instance if the variable templates are at namespace scope.
http://reviews.llvm.org/D3445
More information about the cfe-commits
mailing list