[PATCH] D13330: Implement __attribute__((unique_instantiation))

[Aaron Ballman via cfe-commits]

On Thu, Oct 1, 2015 at 1:09 PM, Keno Fischer
<kfischer at college.harvard.edu> wrote:
> loladiro added a comment.
>
> Thanks for the quick review.
>
>
> ================
> Comment at: include/clang/Basic/Attr.td:1462
> @@ +1461,3 @@
> +def UniqueInstantiation : InheritableAttr {
> +  let Spellings = [GCC<"unique_instantiation">];
> +  let Subjects = SubjectList<[CXXRecord]>;
> ----------------
> aaron.ballman wrote:
>> This is not a GCC attribute, so it should not be spelled as such. Since this only applies to C++ code, I would recommend a C++11 spelling (in the clang namespace). If you think this is something C++03 and earlier code would really benefit from, then you could also add a GNU-style spelling.
> No, this is C++11 only. Will change the spelling.
>
> ================
> Comment at: include/clang/Basic/Attr.td:1464
> @@ +1463,3 @@
> +  let Subjects = SubjectList<[CXXRecord]>;
> +  let Documentation = [Undocumented];
> +}
> ----------------
> aaron.ballman wrote:
>> Please, no undocumented attributes.
> Will document.
>
> ================
> Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
> @@ -2442,1 +2442,3 @@
>    ":must be between 1 and %2}2">;
> +def err_unique_instantiation_wrong_decl : Error<
> +  "unique_instantiation attribute on something that is not a explicit template declaration or instantiation.">;
> ----------------
> aaron.ballman wrote:
>> Would this make more sense as an option in warn_attribute_wrong_decl_type instead? (there's an err_ version as well if you wish to keep this an error).
>>
>> Also, please ensure that the attribute is quoted in the diagnostic -- it makes things less confusing for the user.
> Ok, so should I add an "explicit template instantiations" option to that err?

"explicit instantiation definition" seems reasonable if this is
limited to only applying to definitions and not declarations. You
could also automated the checking for this by updating
ClangAttrEmitter.cpp, but I don't think that's a requirement for this
patch.

> ================
> Comment at: lib/AST/ASTContext.cpp:8244
> @@ -8242,2 +8243,3 @@
>    case TSK_ExplicitInstantiationDefinition:
> -    return GVA_StrongODR;
> +    CTSD = dyn_cast<ClassTemplateSpecializationDecl>(FD->getDeclContext());
> +    if (!CTSD || !CTSD->hasAttr<UniqueInstantiationAttr>())
> ----------------
> aaron.ballman wrote:
>> I think this would be easier to read (and not have to hoist a declaration out of the switch) as:
>>
>> ```
>> if (const auto *CTSD = dyn_cast<>()) {
>>   if (CTSD->hasAttr<>())
>>     return GVA_StrongExternal;
>> }
>> return GVA_StrongODR;
>> ```
>>
> Ok.
>
> ================
> Comment at: lib/Sema/SemaDeclAttr.cpp:4539
> @@ +4538,3 @@
> +    // by an ExplicitInstantiationDeclaration.
> +    if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
> +      if (!CTSD->getPreviousDecl())
> ----------------
> aaron.ballman wrote:
>> Why is this required as part of the feature design? It seems restrictive.
> This was part of Doug's original Spec, so I implemented it:
>
>
>
>> A unique explicit instantiation definition shall follow an explicit
>> instantiation declaration of the same entity. [//Note//: this
>> requirement encourages a programming style that uses unique explicit
>> instantiation declarations (typically in a header) to suppress
>> implicit instantiations of a template or its members, so that the
>> unique explicit instantiation definition of that template or its members
>> is unique. //- end note//]
>
> I think that makes a decent amount of sense, since you really want to avoid the case where some translation units don't see the extern template declaration.

Okay, that makes sense to me as well. Thank you for pointing that out.
You may want to include this (at least in part) in the comments.

> ================
> Comment at: lib/Sema/SemaDeclAttr.cpp:4546
> @@ +4545,3 @@
> +    // have performed the same check on the previous declaration here.
> +    CXXRecordDecl *Previous = CTSD->getPreviousDecl();
> +    if (Previous) {
> ----------------
> aaron.ballman wrote:
>> Is this something that can be handled by mergeDeclAttribute()? I'm not certain how that interplays with templates specifically, but usually we do this sort of logic within a Sema::mergeFooAttr() function.
> Hmm, I'm not sure, the goal of this is to ensure that all declarations and definitions of this extern template have the attribute set. It's not really `merging` per se. Though I suppose it could be made to fit in that framework.

Merging is usually the place where we handle "what do other
declarations and definitions of this thing do", which is why it came
to mind. If it turns out that it's an inappropriate place for it, it
would be good to know why.

~Aaron

>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13330
>
>
>