On Tue, Feb 14, 2017 at 4:21 PM Mehdi AMINI via Phabricator via cfe-commits
wrote:
> mehdi_amini added a comment.
>
> In https://reviews.llvm.org/D13330#582607, @rsmith wrote:
>
> > I think this attribute is poorly named. Explicit instantiation
> definitions are *already* required to be globally
mehdi_amini added a comment.
In https://reviews.llvm.org/D13330#582607, @rsmith wrote:
> 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
loladiro planned changes to this revision.
loladiro added a comment.
Thanks for the review! Sorry, it's been a while since I wrote this code, so I'm
not fluent in all the details, but I've replied below. I am not particularly
partial to the name, so whatever you feel is best is ok with me.
==
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
loladiro updated this revision to Diff 76256.
loladiro added a comment.
Fix for the corner case I found and add it as a test.
Repository:
rL LLVM
https://reviews.llvm.org/D13330
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.t
loladiro added a comment.
I meant
`template __attribute__((unique_instantiation)) int *
static_separate_template::a_static_field;`
of course, though we probably need a better diagnostic for the other spelling
(which applies the attribute to the static_separate_template). I'll look
into adding
loladiro added a comment.
I found a bug in this which I need to fix, but while I'm at it, in doing more
testing on this, I came across the following corner case:
template
struct static_separate_template {
typedef T element;
static T *a_static_field;
};
extern template struct
loladiro set the repository for this revision to rL LLVM.
loladiro updated this revision to Diff 76102.
loladiro added a comment.
Rebased on current master.
Repository:
rL LLVM
https://reviews.llvm.org/D13330
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/cl
majnemer added a comment.
I think this looks good but I'd like @rsmith to take a look.
https://reviews.llvm.org/D13330
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
loladiro added a comment.
I came across a situation again where this would be useful to have. I know this
was approved, but looking it looks like I wanted @majnemer to have another look.
https://reviews.llvm.org/D13330
___
cfe-commits mailing list
loladiro added a comment.
Bumping this again.
http://reviews.llvm.org/D13330
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
loladiro updated this revision to Diff 43218.
loladiro added a comment.
Rebased
http://reviews.llvm.org/D13330
Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/AttributeList.h
include/clang/Sema/Sema.h
l
loladiro added a comment.
@majnemer Do you like the new approach? Is there anything else to be done here?
http://reviews.llvm.org/D13330
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
loladiro updated this revision to Diff 42137.
loladiro added a comment.
Propagate unique instantiation attribute to children rather than later trying
to look through the DeclContexts to see if we're contained in one that has the
attribute.
http://reviews.llvm.org/D13330
Files:
include/clang
loladiro updated this revision to Diff 42132.
loladiro added a comment.
Address David's concern about inner classes. David also suggested on IRC to
propagate
the unique instantiation attribute down rather than walking the context chain to
check for the attribute. I'll try that out, but wanted to
loladiro added inline comments.
Comment at: lib/AST/ASTContext.cpp:8257-8266
@@ -8256,1 +8256,12 @@
+bool ASTContext::containedInUniqueInstantiation(const Decl *D) {
+ const RecordDecl *RD;
+ while ((RD = dyn_cast(D->getDeclContext( {
+auto *CTSD = dyn_cast(RD);
+i
majnemer added inline comments.
Comment at: lib/AST/ASTContext.cpp:8257-8266
@@ -8256,1 +8256,12 @@
+bool ASTContext::containedInUniqueInstantiation(const Decl *D) {
+ const RecordDecl *RD;
+ while ((RD = dyn_cast(D->getDeclContext( {
+auto *CTSD = dyn_cast(RD);
+i
loladiro updated this revision to Diff 42100.
loladiro added a comment.
Address stylistic concerns brought up by David
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds
loladiro added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:1736-1755
@@ -1722,2 +1735,22 @@
+
+// Explicit template definition (in exactly ONE .cpp file)
+template struct __attribute__((unique_instantiation)) my_template;
+
+
+When the unique_instantiation
majnemer added inline comments.
Comment at: include/clang/Basic/AttrDocs.td:1736-1755
@@ -1722,2 +1735,22 @@
+
+// Explicit template definition (in exactly ONE .cpp file)
+template struct __attribute__((unique_instantiation)) my_template;
+
+
+When the unique_instantiation
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, but please wait for David to sign off before committing.
http://reviews.llvm.org/D13330
___
cfe-commits mailing list
cfe-comm
loladiro added a comment.
Bump, is there anything else that's needed here?
http://reviews.llvm.org/D13330
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
loladiro updated this revision to Diff 41265.
loladiro added a comment.
Add support for variable templates
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
includ
loladiro updated this revision to Diff 41261.
loladiro added a comment.
Rebased and made the small suggested changes to the test cases.
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/Di
aaron.ballman added a comment.
Modulo the question you and David are discussing about variable templates (for
which I don't have an answer handy), I just have a few small testing nits.
Comment at: test/SemaCXX/unique-instantiations.cpp:24
@@ +23,3 @@
+template struct foo5;
loladiro updated this revision to Diff 39627.
loladiro updated the summary for this revision.
loladiro added a comment.
Address review feedback regarding diagnostic wording/expand tests to full text
of diagnostic.
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include
loladiro added inline comments.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+ let Documentation = [UniqueInstantiationDocs];
loladiro
aaron.ballman added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2456
@@ -2450,1 +2455,3 @@
+def err_unique_instantiation_not_previous : Error<
+ "'unique_instantiation' attribute must be specified for all declarations and
definitions of this explicit
loladiro added inline comments.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+ let Documentation = [UniqueInstantiationDocs];
majnemer
majnemer added inline comments.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+ let Documentation = [UniqueInstantiationDocs];
They wor
loladiro set the repository for this revision to rL LLVM.
loladiro updated this revision to Diff 38039.
loladiro added a comment.
Address review comments.
Repository:
rL LLVM
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/
loladiro added inline comments.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+ let Documentation = [UniqueInstantiationDocs];
majnemer
majnemer added a subscriber: majnemer.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag>;
+ let Documentation = [UniqueInstantiationDocs];
W
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 t
loladiro updated this revision to Diff 37555.
loladiro added a comment.
Address review comments and clang-format.
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Basic/DiagnosticSemaKinds.td
loladiro 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
aaron.ballman wr
aaron.ballman added inline comments.
Comment at: include/clang/Basic/Attr.td:1463
@@ +1462,3 @@
+ let Spellings = [GNU<"unique_instantiation">];
+ let Subjects = SubjectList<[Function, CXXRecord], ErrorDiag,
"ExpectedFunctionOrClass">;
+ let Documentation = [UniqueInstantiatio
loladiro added a comment.
Ok, I have tested this more extensively now. I'm happy with the results. Here's
a patch that applies this to LLVM for example:
https://gist.github.com/Keno/79b08a4b187c4d950dd0
Before:
$llvm-objdump -weak-bind libLLVM-3.8svn.dylib | wc -l
300
After:
$llvm
loladiro updated this revision to Diff 36476.
loladiro added a comment.
Also set the correct linkage on vtables of classes with the new attribute.
http://reviews.llvm.org/D13330
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/cla
loladiro removed rL LLVM as the repository for this revision.
loladiro updated this revision to Diff 36459.
loladiro added a comment.
Rebased, added support for unique_instantiation on explicit function templates
and fix the case where one record is embedded in another and the outer is
explicitl
loladiro added a comment.
Thoughts on allowing this attribute to be specified on the templated class
itself, with the intention of never allowing any implicit instantiation? As an
example, consider SymbolTableListTraits in LLVM. It can only ever be used as an
explicit instantiation (because the
On Fri, Oct 2, 2015 at 1:26 AM, Keno Fischer
wrote:
> loladiro added inline comments.
>
>
> Comment at: include/clang/Basic/Attr.td:1462
> @@ +1461,3 @@
> +def UniqueInstantiation : InheritableAttr {
> + let Spellings = [GCC<"unique_instantiation">];
> + let Subjects = SubjectLi
loladiro updated this revision to Diff 36332.
loladiro added a comment.
Address review comments. I had to add a special case to
checkNewAttributesAfterDef if we want to use attribute merging for explicit
template instantiations, because the Microsoft ABI allows adding dll attributes
to the expl
loladiro added inline comments.
Comment at: include/clang/Basic/Attr.td:1462
@@ +1461,3 @@
+def UniqueInstantiation : InheritableAttr {
+ let Spellings = [GCC<"unique_instantiation">];
+ let Subjects = SubjectList<[CXXRecord]>;
loladiro wrote:
> aaron.ballman wr
On Thu, Oct 1, 2015 at 1:09 PM, Keno Fischer
wrote:
> 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">];
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.bal
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 = Subjec
loladiro created this revision.
loladiro added a reviewer: doug.gregor.
loladiro added a subscriber: cfe-commits.
loladiro set the repository for this revision to rL LLVM.
This implements a proposal by Doug Gregor on cfe-dev a couple of years ago, to
allow the compiler to emit strong symbols for
48 matches
Mail list logo
