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