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

[David Majnemer via cfe-commits]

majnemer added inline comments.

================
Comment at: include/clang/Basic/AttrDocs.td:1736-1755
@@ -1722,2 +1735,22 @@
+
+    // Explicit template definition (in exactly ONE .cpp file)
+    template struct __attribute__((unique_instantiation)) my_template<int>;
+
+
+When the unique_instantiation attribute is specified on an explicit template
+instantiation, the compiler is given license to emit strong symbols for
+this specific explicit template instantiation.
+
+If the attribute is present on one such definition or declaration for a given
+entity, it must be present on all.
+
+Note that to ensure correct execution the user MUST make certain that no
+other translation unit has an implicit instantiation of the same entity. In
+particular this means that any usage of the entity has to be preceeded by an
+appropriate explicit template declaration or definition.
 
+It is thus recommended that explicit template declarations are placed in headers
+to suppress any potential implicit instantiation of the entity. In order to
+encourage this programming style, any explicit template definition with this
+attribute MUST be preceeded by an appropriate declaration.
   }];
----------------
Instead of using all-caps for emphasis in "ONE" and "MUST", why not bold the text instead?  It would catch the eye a little faster.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2533-2534
@@ -2531,1 +2532,4 @@
   ":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+  "'unique_instantiation' attribute only applies to an explicit template declaration or instantiation">;
+def err_unique_instantiation_no_declaration : Error<
----------------
I can't seem to see any users of `err_unique_instantiation_wrong_decl`.

================
Comment at: lib/AST/ASTContext.cpp:8283-8286
@@ +8282,6 @@
+        Context.containedInUniqueInstantiation(FD)) {
+      // We return GVA_StrongExternal here, instead of going through the logic
+      // below, because even if the definition is available inline, since the
+      // source specified  an explicit template instantiation, we want to make
+      // the symbol available.
+      return GVA_StrongExternal;
----------------
There are a lot of commas here, making it a little hard to read the justification.

Perhaps something along the lines of:

> This translation unit is responsible for emitting a unique instantiation of this function regardless of whether or not the function is marked inline.

================
Comment at: lib/CodeGen/CGVTables.cpp:744-747
@@ -743,2 +743,6 @@
       case TSK_ExplicitInstantiationDefinition:
+        // If the key function has strong linkage (say due to
+        // UniqueInstantiationAttr), the VTable should too.
+        if (Context.containedInUniqueInstantiation(keyFunction))
+          return llvm::GlobalVariable::ExternalLinkage;
         return !Context.getLangOpts().AppleKext ?
----------------
What if the key function is not contained within a record which is marked as `UniqueInstantiationAttr` but the key function itself is marked `UniqueInstantiationAttr`?


http://reviews.llvm.org/D13330