loladiro added a comment.
Thanks for the quick review.
================
Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+ let Spellings = [GCC<"unique_instantiation">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
aaron.ballman wrote:
> This is not a GCC attribute, so it should not be spelled as such. Since this
> only applies to C++ code, I would recommend a C++11 spelling (in the clang
> namespace). If you think this is something C++03 and earlier code would
> really benefit from, then you could also add a GNU-style spelling.
No, this is C++11 only. Will change the spelling.
================
Comment at: include/clang/Basic/Attr.td:1464
@@ +1463,3 @@
+ let Subjects = SubjectList<[CXXRecord]>;
+ let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> Please, no undocumented attributes.
Will document.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2443
@@ -2442,1 +2442,3 @@
":must be between 1 and %2}2">;
+def err_unique_instantiation_wrong_decl : Error<
+ "unique_instantiation attribute on something that is not a explicit template
declaration or instantiation.">;
----------------
aaron.ballman wrote:
> Would this make more sense as an option in warn_attribute_wrong_decl_type
> instead? (there's an err_ version as well if you wish to keep this an error).
>
> Also, please ensure that the attribute is quoted in the diagnostic -- it
> makes things less confusing for the user.
Ok, so should I add an "explicit template instantiations" option to that err?
================
Comment at: lib/AST/ASTContext.cpp:8244
@@ -8242,2 +8243,3 @@
case TSK_ExplicitInstantiationDefinition:
- return GVA_StrongODR;
+ CTSD = dyn_cast<ClassTemplateSpecializationDecl>(FD->getDeclContext());
+ if (!CTSD || !CTSD->hasAttr<UniqueInstantiationAttr>())
----------------
aaron.ballman wrote:
> I think this would be easier to read (and not have to hoist a declaration out
> of the switch) as:
>
> ```
> if (const auto *CTSD = dyn_cast<>()) {
> if (CTSD->hasAttr<>())
> return GVA_StrongExternal;
> }
> return GVA_StrongODR;
> ```
>
Ok.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4539
@@ +4538,3 @@
+ // by an ExplicitInstantiationDeclaration.
+ if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
+ if (!CTSD->getPreviousDecl())
----------------
aaron.ballman wrote:
> Why is this required as part of the feature design? It seems restrictive.
This was part of Doug's original Spec, so I implemented it:
> A unique explicit instantiation definition shall follow an explicit
> instantiation declaration of the same entity. [//Note//: this
> requirement encourages a programming style that uses unique explicit
> instantiation declarations (typically in a header) to suppress
> implicit instantiations of a template or its members, so that the
> unique explicit instantiation definition of that template or its members
> is unique. //- end note//]
I think that makes a decent amount of sense, since you really want to avoid the
case where some translation units don't see the extern template declaration.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4546
@@ +4545,3 @@
+ // have performed the same check on the previous declaration here.
+ CXXRecordDecl *Previous = CTSD->getPreviousDecl();
+ if (Previous) {
----------------
aaron.ballman wrote:
> Is this something that can be handled by mergeDeclAttribute()? I'm not
> certain how that interplays with templates specifically, but usually we do
> this sort of logic within a Sema::mergeFooAttr() function.
Hmm, I'm not sure, the goal of this is to ensure that all declarations and
definitions of this extern template have the attribute set. It's not really
`merging` per se. Though I suppose it could be made to fit in that framework.
Repository:
rL LLVM
http://reviews.llvm.org/D13330
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
