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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+  let Spellings = [GNU<"unique_instantiation">];
+  let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag, "ExpectedFunctionOrClass">;
+  let Documentation = [UniqueInstantiationDocs];
----------------
This one should be handled in ClangAttrEmitter.cpp; I can see it being used far more frequently than ExpectedExplicitInstantiation, and all the machinery should already be in place to handle it (in CalculateDiagnostic()).

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

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

================
Comment at: lib/Sema/SemaDecl.cpp:2177
@@ -2176,1 +2176,3 @@
 
+static void checkUniqueInstantiationAttrs(Sema &S, const Decl *New, const Decl *Old) {
+  // Check that any previous definitions also had this attribute set.
----------------
This function has some formatting issues. Also, I feel like some of the code duplication could be removed.
```
if (old == new)
  return;

Loc = Old->hasAttr ? new->getLocStart() : new->getAttr->getLocation();
S.Diag(Loc, diag::blah);
S.Diag(Old->getLocStart(0, blah);
```

================
Comment at: lib/Sema/SemaDecl.cpp:2285
@@ +2284,3 @@
+  // here.
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(New)) {
+    TemplateSpecializationKind kind = CTSD->getSpecializationKind();
----------------
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. 

================
Comment at: lib/Sema/SemaDeclAttr.cpp:4568
@@ +4567,3 @@
+  }
+  S.Diag(Attr.getLoc(),
+         diag::err_attribute_wrong_decl_type)
----------------
The formatting of this line seems a bit weird -- did clang-format do this?

================
Comment at: lib/Sema/SemaTemplate.cpp:7855
@@ +7854,3 @@
+    TSK_ExplicitInstantiationDeclaration;
+  bool HadUniqueInstantiation = Specialization->hasAttr<UniqueInstantiationAttr>();
+  SourceLocation OldUniqueInstantiationLoc;
----------------
Instead of hasAttr<> followed by getAttr<>, better to just call getAttr<> and handle null.

================
Comment at: lib/Sema/SemaTemplate.cpp:7868
@@ -7862,1 +7867,3 @@
 
+  if (Specialization->hasAttr<UniqueInstantiationAttr>()) {
+    if (TSK == TSK_ExplicitInstantiationDefinition && !HadDeclaration)
----------------
Same comment here.

================
Comment at: lib/Sema/SemaTemplate.cpp:7877
@@ +7876,3 @@
+  } else if (HadUniqueInstantiation) {
+    Diag(D.getIdentifierLoc(),diag::err_unique_instantiation_not_previous);
+    Diag(OldUniqueInstantiationLoc,diag::note_previous_explicit_instantiation);
----------------
Formatting; I would recommend running clang-format over the patch to catch these sort of things.


http://reviews.llvm.org/D13330