On Wed, Jul 27, 2011 at 2:08 PM, Douglas Gregor <[email protected]> wrote: > > 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 >
Hi Doug, thanks for the review. I submitted the feature in r137573. I'll finish the serialization with a test case in a followup patch. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
