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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added reviewers: rsmith, aaron.ballman.

================
Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+  let Spellings = [GCC<"unique_instantiation">];
+  let Subjects = SubjectList<[CXXRecord]>;
----------------
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.

================
Comment at: include/clang/Basic/Attr.td:1464
@@ +1463,3 @@
+  let Subjects = SubjectList<[CXXRecord]>;
+  let Documentation = [Undocumented];
+}
----------------
Please, no undocumented attributes.

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

================
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>())
----------------
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;
```


================
Comment at: lib/AST/ASTContext.cpp:8353
@@ -8342,2 +8352,3 @@
   case TSK_ExplicitInstantiationDefinition:
-    return GVA_StrongODR;
+    CTSD = dyn_cast<ClassTemplateSpecializationDecl>(VD->getDeclContext());
+    if (!CTSD || !CTSD->hasAttr<UniqueInstantiationAttr>())
----------------
Similar suggestion here.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4535
@@ +4534,3 @@
+                                      const AttributeList &Attr) {
+  ClassTemplateSpecializationDecl *CTSD;
+  if ((CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))) {
----------------
The declaration does not need to be hoisted here.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4539
@@ +4538,3 @@
+    // by an ExplicitInstantiationDeclaration.
+    if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
+      if (!CTSD->getPreviousDecl())
----------------
Why is this required as part of the feature design? It seems restrictive.

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


Repository:
  rL LLVM

http://reviews.llvm.org/D13330