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

[Richard Smith via cfe-commits]

rsmith added a comment.

I think this attribute is poorly named. Explicit instantiation definitions are *already* required to be globally unique; see [temp.spec]/5.1:

"For a given template and a given set of template-arguments, an explicit instantiation definition shall appear at most once in a program"

The actual meaning of the attribute is that an explicit instantiation declaration will appear before any use that might trigger implicit instantiation. To that end, something like `__attribute__((declared_before_all_uses))` might be clearer.



================
Comment at: include/clang/Basic/AttrDocs.td:2321-2323
+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.
----------------
The primary description of the attribute should specify what it means, not the consequences of that meaning. In this case, the meaning is that the programmer is guaranteeing to the compiler that an explicit instantiation precedes all implicit instantiations, and the consequence is that we can use strong symbols.


================
Comment at: include/clang/Basic/AttrDocs.td:1736-1755
+    // 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.
----------------
loladiro wrote:
> majnemer wrote:
> > 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.
> Sure! I assume, this follows standard RST conventions where ** is bold?
Yes, this documentation allows arbitrary RST markup. But I don't see the need for strong emphasis here.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
+def err_unique_instantiation_not_previous : Error<
+  "'unique_instantiation' attribute must be specified for all declarations and definitions of this explicit template instantiation">;
 
----------------
loladiro wrote:
> aaron.ballman wrote:
> > > They are checking for two different conditions in the spec. One requires that all explicit template instantiations with this attribute have a declaration, the other that every declaration/definition has the attribute.
> > 
> > Okay, I see now what this diagnostic is attempting to convey. I think it should read:
> > ```
> > def err_unique_instantiation_no_declaration : Error<
> >   "'unique_instantiation' attribute on an explicit instantiation requires a previous explicit instantiation declaration">;
> > ```
> Sounds good.
This seems like an unnecessarily-strict rule to me. I don't see a reason to disallow:
```
// in header
extern template struct __attribute__((whatever_its_called)) X<int>;
```
```
// in source file
template struct X<int>;
```
There doesn't appear to be any safety or correctness concern here. In general, it seems like the attribute is only promising that the explicit instantiation declaration precedes any other instantiation, so the only necessary rule would seem to be: if the attribute is present on any explicit instantiation, the first point of instantiation must be an explicit instantiation declaration that has the attribute.


================
Comment at: lib/AST/ASTContext.cpp:8789
 
-  return GVA_DiscardableODR;
+  return  GVA_DiscardableODR;
 }
----------------
revert this change


================
Comment at: lib/Sema/SemaDecl.cpp:2396-2397
+  // Explicit template instantiations need special handling because in certain
+  // ABIs explicit template definitions may add attributes over explicit
+  // template declarations. In clang getDefinition() will get the
+  // ClassTemplateSpecializationDecl associated with the class template
----------------
I assume you mean "explicit instantiation" rather than "explicit template" here (2x).


================
Comment at: lib/Sema/SemaDecl.cpp:2398-2399
+  // template declarations. In clang getDefinition() will get the
+  // ClassTemplateSpecializationDecl associated with the class template
+  // declaration, so we'd give incorrect warnings here.
+  if (auto *CTSD = dyn_cast<ClassTemplateSpecializationDecl>(New)) {
----------------
I have no idea what you mean by this. Did you mean "associated with the explicit instantiation declaration" or something like that?


================
Comment at: lib/Sema/SemaDeclAttr.cpp:5400
+      if (!CTSD->getPreviousDecl())
+        S.Diag(Attr.getLoc(), diag::err_unique_instantiation_no_declaration);
+    }
----------------
Why is this rule only applied to explicit instantiations of class templates, and not to function or variable templates?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:5666-5685
+/// \brief Check if the new class needs a unique instantiation attribute
+/// based on whether its containing function or class has it
+void Sema::checkClassLevelUniqueInstantiation(CXXRecordDecl *Record) {
+  Decl *D = Record;
+  if (!D || D->hasAttr<UniqueInstantiationAttr>())
+    return;
+  while (DeclContext *DC = D ? D->getDeclContext() : nullptr) {
----------------
I don't think this is the right way to handle this case. Note that an explicit instantiation declaration/definition for a class is only an explicit instantiation of those members that are defined at the point where the explicit instantiation appears. It seems like the most consistent behavior is probably to add the attributes to the defined class members as part of the process of explicitly instantiating them, in `InstantiateClassMembers`.


================
Comment at: lib/Sema/SemaTemplate.cpp:8063-8066
+      DiagnoseUniqueInstantiation(*this, D, TSK, HadDeclaration,
+                                  Prev->getAttr<UniqueInstantiationAttr>(),
+                                  OldUniqueInstantiation,
+                                  OldUniqueInstantiationLoc);
----------------
Do we need this to be here as a special case, duplicated for each kind of decl that can have these attributes, rather than handling it using the normal attribute-handling mechanism in SemaDeclAttr? If so, why?


Repository:
  rL LLVM

https://reviews.llvm.org/D13330