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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
