[cfe-commits] [PATCH][MS][Review request] - Microsoft function specialization at scope scope

[Francois Pichet]

On Wed, Jul 27, 2011 at 2:08 PM, Douglas Gregor <dgregor at apple.com> 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 <dgregor at apple.com> 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.