aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added reviewers: rsmith, aaron.ballman.
================
Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+ let Spellings = [GCC<"unique_instantiation">];
+ let Subjects = SubjectList<[CXXRecord]>;
----------------
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.
================
Comment at: include/clang/Basic/Attr.td:1464
@@ +1463,3 @@
+ let Subjects = SubjectList<[CXXRecord]>;
+ let Documentation = [Undocumented];
+}
----------------
Please, no undocumented attributes.
================
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.">;
----------------
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.
================
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>())
----------------
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;
```
================
Comment at: lib/AST/ASTContext.cpp:8353
@@ -8342,2 +8352,3 @@
case TSK_ExplicitInstantiationDefinition:
- return GVA_StrongODR;
+ CTSD = dyn_cast<ClassTemplateSpecializationDecl>(VD->getDeclContext());
+ if (!CTSD || !CTSD->hasAttr<UniqueInstantiationAttr>())
----------------
Similar suggestion here.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4535
@@ +4534,3 @@
+ const AttributeList &Attr) {
+ ClassTemplateSpecializationDecl *CTSD;
+ if ((CTSD = dyn_cast<ClassTemplateSpecializationDecl>(D))) {
----------------
The declaration does not need to be hoisted here.
================
Comment at: lib/Sema/SemaDeclAttr.cpp:4539
@@ +4538,3 @@
+ // by an ExplicitInstantiationDeclaration.
+ if (CTSD->getSpecializationKind() == TSK_ExplicitInstantiationDefinition) {
+ if (!CTSD->getPreviousDecl())
----------------
Why is this required as part of the feature design? It seems restrictive.
================
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) {
----------------
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.
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
