On Jul 9, 2011, at 5:19 PM, Francois Pichet wrote:
> Thanks for the review, here is patch #2 with some changes:
>
> On Fri, Jun 24, 2011 at 11:18 AM, Douglas Gregor <[email protected]> wrote:
>>
>> On Jun 16, 2011, at 6:26 PM, Francois Pichet wrote:
>>
>>
>> Index: include/clang/Basic/DiagnosticSemaKinds.td
>> ===================================================================
>> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 133213)
>> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
>> @@ -1867,6 +1867,9 @@
>>
>> "explicit specialization of %0 in function scope">;
>> def err_template_spec_decl_class_scope : Error<
>> "explicit specialization of %0 in class scope">;
>> +def warn_template_spec_decl_class_scope : ExtWarn<
>> + "explicit specialization of %0 in class scope in a Microsoft extension">,
>> + InGroup<Microsoft>;
>>
>> This should say "is a Microsoft extension".
>
> Done
>
>>
>> Index: lib/Sema/SemaDecl.cpp
>> ===================================================================
>> --- lib/Sema/SemaDecl.cpp (revision 133213)
>> +++ lib/Sema/SemaDecl.cpp (working copy)
>> @@ -1688,9 +1688,13 @@
>>
>> // Preserve triviality.
>> NewMethod->setTrivial(OldMethod->isTrivial());
>>
>> + // MSVC allows explicit template specialization at class scope.
>> + bool IsMSExplicitSpecialization = getLangOptions().Microsoft &&
>> +
>> NewMethod->isFunctionTemplateSpecialization();
>> bool isFriend = NewMethod->getFriendObjectKind();
>>
>> - if (!isFriend && NewMethod->getLexicalDeclContext()->isRecord()) {
>> + if (!isFriend && NewMethod->getLexicalDeclContext()->isRecord() &&
>> + !IsMSExplicitSpecialization) {
>> // -- Member function declarations with the same name and the
>> // same parameter types cannot be overloaded if any of them
>> // is a static member function declaration.
>> @@ -4649,6 +4653,17 @@
>>
>> } else if (isFunctionTemplateSpecialization) {
>> if (CurContext->isDependentContext() && CurContext->isRecord()
>> && !isFriend) {
>> + if (getLangOptions().Microsoft) {
>> + ClassScopeFunctionSpecializationDecl *NewSpec =
>> + ClassScopeFunctionSpecializationDecl::Create(
>> + Context, CurContext,
>> SourceLocation(),
>> + cast<CXXMethodDecl>(NewFD));
>> + CurContext->addDecl(NewSpec);
>> + Redeclaration = true;
>> + NewFD->setInvalidDecl();
>> + return NewFD;
>> + }
>> +
>> Diag(NewFD->getLocation(),
>> diag::err_function_specialization_in_class)
>> << NewFD->getDeclName();
>> NewFD->setInvalidDecl();
>>
>> We shouldn't perform an early-exit here, because it means that we're missing
>> out on a lot of semantic analysis, such as adding attributes and checking
>> the qualification. Can we just make a note that this is a class-scope
>> function specialization, do all of the extra checking, and then build the
>> ClassScopeFunctionSpecializationDecl at the end?
>>
>> Also, I think it'd be cleaner if we always used
>> ClassScopeFunctionSpecializationDecl to handle class-scope function
>> specializations, regardless of whether we're in Microsoft mode. It would be
>> great if the only check for getLangOptions().Microsoft was to decide whether
>> to emit the ExtWarn vs. the Error. Plus, we'll get better testing coverage
>> that way :)
>>
>
> Ok done too.
>
>> Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
>> ===================================================================
>> --- lib/Sema/SemaTemplateInstantiateDecl.cpp (revision 133213)
>> +++ lib/Sema/SemaTemplateInstantiateDecl.cpp (working copy)
>> @@ -1890,6 +1890,89 @@
>>
>> return UD;
>> }
>>
>> +Decl *TemplateDeclInstantiator::VisitClassScopeFunctionSpecializationDecl(
>> + ClassScopeFunctionSpecializationDecl
>> *Decl) {
>>
>> There's a lot of redundancy between this method and
>> TemplateDeclInstantiator::VisitCXXMethodDecl(). Did you try passing
>> additional state to VisitCXXMethodDecl, so that we can re-use (and tweak)
>> the code there rather than copying most of the code?
>>
>> This part in particular confuses me:
>>
>> +
>> + // Instantiate NewFD.
>> + SemaRef.InstantiateFunctionDefinition(SourceLocation(), NewFD, false,
>> + false, false);
>>
>> Why are we instantiating the function definition? We shouldn't need to do
>> that.
>>
>
> Ok I removed the duplication and added a flag to VisitCXXMethodDecl.
> This patch is different in that the instantiation is now done properly
> at the end of the translation unit like any others.
Great!
> This add some small complexity (some
> getClassScopeSpecializationPattern checking here and there) to not
> bail out because we are dealing with a TSK_ExplicitSpecialization at
> instantiation time.
Sure, that makes sense.
> Also one potentially controversial aspect of this patch is that it
> adds a new pointer (ClassScopeSpecializationPattern) to the
> FunctionTemplateSpecializationInfo class. This new pointer is used to
> retrieved the function pattern during the instantiation of a class
> scope explicit specialization. So there is a 4-byte added for a rarely
> used feature. Not sure if that's acceptable.
How about turning the "FunctionDecl *Function" into a
PointerIntPair<FunctionDecl*, 1, bool>, where the boolean value indicates
whether ClassScopeSpecializationPattern will be available at address this+1?
This is our preferred way to optimize away storage that isn't needed in the
common case.
Index: lib/Sema/SemaDecl.cpp
===================================================================
--- lib/Sema/SemaDecl.cpp (revision 134851)
+++ lib/Sema/SemaDecl.cpp (working copy)
@@ -1707,9 +1707,16 @@
// Preserve triviality.
NewMethod->setTrivial(OldMethod->isTrivial());
+ // MSVC allows explicit template specialization at class scope:
+ // 2 CXMethodDecls refering to the same function will be injected.
+ // We don't want to redeclartion error.
There are a number of typos in this comment.
@@ -4182,6 +4189,7 @@
FunctionTemplateDecl *FunctionTemplate = 0;
bool isExplicitSpecialization = false;
bool isFunctionTemplateSpecialization = false;
+ bool isDependantClassScopeExplicitSpecialization = false;
How about "isDependentClassScopeExplicitSpecialization"
+ // Here we have an function template explicit specialization at class scope.
+ // The actually specialization will be postponed to template instatiation
+ // time via the ClassScopeFunctionSpecializationDecl node.
+ if (isDependantClassScopeExplicitSpecialization) {
+ ClassScopeFunctionSpecializationDecl *NewSpec =
+ ClassScopeFunctionSpecializationDecl::Create(
+ Context, CurContext, SourceLocation(),
+ cast<CXXMethodDecl>(NewFD));
+ CurContext->addDecl(NewSpec);
+ // FIXME: This is hackish: set NewFD to invalid and Redeclaration to true,
+ // to prevent NewFD from been pushed into scope.
+ NewFD->setInvalidDecl();
+ Redeclaration = true;
+ }
Why not return return NewSpec, and have the caller recognize that it shouldn't
try to make ClassScopeFunctionSpecializationDecls visible? (Since they aren't
NamedDecls anyway).
@@ -1514,7 +1516,7 @@
: Method);
if (isFriend)
Record->makeDeclVisibleInContext(DeclToAdd);
- else
+ else if (!IsClassScopeSpecialization)
Owner->addDecl(DeclToAdd);
}
It seems like it should be okay to addDecl the class scope specializations. Did
that break something?
Index: lib/Serialization/ASTReaderDecl.cpp
===================================================================
--- lib/Serialization/ASTReaderDecl.cpp (revision 134851)
+++ lib/Serialization/ASTReaderDecl.cpp (working copy)
@@ -1576,6 +1576,10 @@
D = ClassTemplatePartialSpecializationDecl::Create(*Context,
Decl::EmptyShell());
break;
+ case DECL_CLASS_SCOPE_FUNCTION_SPECIALIZATION:
+ D = ClassScopeFunctionSpecializationDecl::Create(*Context,
+ Decl::EmptyShell());
+ break;
case DECL_FUNCTION_TEMPLATE:
D = FunctionTemplateDecl::Create(*Context, Decl::EmptyShell());
break;
You'll also need to add a VisitClassScopeFunctionSpecializationDecl to the
reader, and add writing support. A test case exercising PCH would be greatly
appreciated.
This is looking great!
- Doug
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
