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

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 20 17:16:28 PDT 2015


aaron.ballman 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
----------------
> I don't think there is anything that can be really done here.

That's unfortunate, but I think you may be right. Blech.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2452
@@ +2451,3 @@
+def err_unique_instantiation_wrong_decl : Error<
+  "unique_instantiation attribute on something that is not a explicit template declaration or instantiation.">;
+def err_unique_instantiation_no_declaration : Error<
----------------
Please quote 'unique_instantiation' in the diagnostic (here and in the subsequent uses).

Instead of "something", let's use "a declaration"?

"a explicit template" should be "an explicit template".

Also, diagnostics are not complete sentences, so should not be capitalized or end with a full stop.

Actually, how about this for a new wording:

"'unique_instantiation' attribute only applies to an explicit template declaration or instantiation"

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+  "The unique_instantiation must be specified on all declarations and definitions of a particular explicit template instantiation.">;
 
----------------
Should say: the 'unique_instantiation' attribute.

Also, I'm not keen on "a particular" as it's very non-specific.

I wonder if we need err_unique_instantiation_no_declaration and err_unique_instantiation_not_previous to be separate diagnostics? Aren't they both effectively saying, 

"a 'unique_instantiation' attribute must be specified for all declarations and definitions of the explicit template instantiation"

================
Comment at: lib/AST/ASTContext.cpp:8246
@@ +8245,3 @@
+    if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
+      if (CTSD->hasAttr<UniqueInstantiationAttr>()) {
+        return true;
----------------
Elide braces

================
Comment at: lib/Sema/SemaDecl.cpp:2184
@@ +2183,3 @@
+
+  Attr *NewAttr = New->getAttr<UniqueInstantiationAttr>();
+  SourceLocation NewLoc = NewAttr ? NewAttr->getLocation() : New->getLocStart();
----------------
hasAttr<> followed by getAttr<> that can be simplified.

================
Comment at: lib/Sema/SemaDecl.cpp:2287
@@ +2286,3 @@
+  // declaration, so we'd give incorrect warnings here.
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(New)) {
+    TemplateSpecializationKind kind = CTSD->getSpecializationKind();
----------------
> 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.

Okay, thanks!

================
Comment at: lib/Sema/SemaTemplate.cpp:7874
@@ +7873,3 @@
+           diag::err_unique_instantiation_no_declaration);
+    else if (HadDeclaration && !OldUniqueInstantiation) {
+      Diag(NewUniqueInstantiation->getLocation(),
----------------
Elide braces


http://reviews.llvm.org/D13330





More information about the cfe-commits mailing list