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

[Keno Fischer via cfe-commits]

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?

================
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.


================
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D13330