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

[Keno Fischer via cfe-commits]

loladiro added inline comments.

================
Comment at: include/clang/Basic/AttrDocs.td:1638
@@ +1637,3 @@
+
+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
----------------
aaron.ballman wrote:
> In the unlikely event the user gets this wrong (lol), is there a way to diagnose this on the backend? The wording here makes it sound like it's possible for this to cause silent miscompiles, which would be unfortunate.
I don't think there is anything that can be really done here. I believe the static linker will link this just fine (there'll be one weak and one strong symbol). The difficulty of course arises with dynamic linkers that won't bother looking for any weak symbols to merge with (at least the OS X one doesn't). I suppose it's not really that different from missing a `weak` attribute on some declarations but not others.

================
Comment at: lib/CodeGen/CGVTables.cpp:744
@@ +743,3 @@
+        // UniqueInstantiationAttr), the VTable should too.
+        keyFunction->dump();
+        if (Context.containedInUniqueInstantiation(keyFunction))
----------------
aaron.ballman wrote:
> Looks like some debugging code accidentally made it in.
Thanks!

================
Comment at: lib/Sema/SemaDecl.cpp:2285
@@ +2284,3 @@
+  // here.
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(New)) {
+    TemplateSpecializationKind kind = CTSD->getSpecializationKind();
----------------
aaron.ballman wrote:
> Isn't this a bit broad? I do agree that we don't want to give incorrect warnings, but it seems like this may inadvertently silence correct warnings as well. If my gut feeling is wrong (which it might be), it would be good to have some test cases confirming that this doesn't silence correct diagnostics. 
As far as I know, this is only called from mergeDeclAttributes, which didn't use to be called on these before this patch. I also don't think any attributes but dllexport or unique_instantiation apply to these declarations and those two both need special processing.


http://reviews.llvm.org/D13330