Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis closed this revision. eugenis added a comment. r252648. Thanks everyone for the very detailed review! Repository: rL LLVM http://reviews.llvm.org/D13925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39751. eugenis added a comment. Rebase. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/AST/Decl.cpp

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added a comment. Richard, are you OK with this? Repository: rL LLVM http://reviews.llvm.org/D13925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

aaron.ballman added a comment. LGTM, thank you! Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4090 @@ +4089,3 @@ + "'internal_linkage' attribute on a non-static local variable is ignored">, + InGroup; + Good catch! Repository: rL LLVM http://revi

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:3407 @@ +3406,3 @@ + : ExpectedVariableOrFunction); +D->dropAttr(); + } aaron.ballman wrote: > Why is this dropping AlwaysInlineAttr instead of re

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39389. eugenis marked an inline comment as done. eugenis added a comment. Added the new warning to a -W group. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:6609 @@ +6608,3 @@ + + if (NewVD->hasLocalStorage() && NewVD->hasAttr()) { +Diag(NewVD->getLocation(), diag::warn_internal_linkage_local_storage); Is there a reason this change cannot go i

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: test/SemaCXX/internal_linkage.cpp:23 @@ +22,3 @@ + +__attribute__((internal_linkage)) void A::f4() {} // expected-error{{'internal_linkage' attribute does not appear on the first declaration of 'f4'}} + Btw, this trigge

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added a comment. In http://reviews.llvm.org/D13925#281349, @eugenis wrote: > Hm, the current implementation allows all of the following: > > void f(int a [[clang::internal_linkage]]) { // 1 > > int b [[clang::internal_linkage]]; // 2 > static int c [[clang::internal_linkage]]; //

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39249. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/SemaDecl.cpp lib/Sema/

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39245. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/SemaDecl.cpp lib/Sema/

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added a comment. How do I check if a Var is a local variable? Comment at: test/CodeGenCXX/attribute_internal_linkage.cpp:35-36 @@ +34,4 @@ +__attribute__((internal_linkage)) void A::f3() { +} + +// Forward declaration w/o an attribute. OK, done. Still al

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

rnk added a comment. This is an interesting test case, though: inline int foo() { static int __attribute__((internal_linkage)) x; return x++; } If foo gets inlined, those call sites will use and update 'x'. If foo is not inlined, one definition of foo will win, and every caller will

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

On Wed, Nov 4, 2015 at 2:20 PM, Evgeniy Stepanov wrote: > eugenis added a comment. > > Hm, the current implementation allows all of the following: > > void f(int a [[clang::internal_linkage]]) { // 1 > > int b [[clang::internal_linkage]]; // 2 > static int c [[clang::internal_linkage]]; //

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added a comment. Hm, the current implementation allows all of the following: void f(int a [[clang::internal_linkage]]) { // 1 int b [[clang::internal_linkage]]; // 2 static int c [[clang::internal_linkage]]; // 3 } I'll fix (1). Is it OK to allow (2) and (3)? The attribute has

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

rsmith added inline comments. Comment at: test/CodeGenCXX/attribute_internal_linkage.cpp:34-35 @@ +33,4 @@ + +__attribute__((internal_linkage)) void A::f3() { +} + We should reject this. We're just causing problems for ourselves if we allow the linkage to change

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I would like to see one more test, just to make sure that a Var subject doesn't also allow it on a parameter: void f(int a [[clang::internal_linkage]]); Aside from that, LGTM!

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: lib/AST/Decl.cpp:635-641 @@ -634,2 +634,9 @@ assert(!isa(D) && "Didn't expect a FieldDecl!"); + for (const DeclContext *DC = D->getDeclContext(); + !isa(DC); DC = DC->getParent()) { +const NamespaceDecl *ND = dyn_cast(DC);

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

On Tue, Nov 3, 2015 at 7:44 PM, Richard Smith wrote: > On Tue, Nov 3, 2015 at 4:37 PM, Aaron Ballman via cfe-commits > wrote: >> >> On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith >> wrote: >> > rsmith added inline comments. >> > >> > >> > Comment at: include/clang/Basic/AttrDocs.

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39135. eugenis marked 2 inline comments as done. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/SemaDecl.cpp lib/Sema/Sem

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

On Tue, Nov 3, 2015 at 4:37 PM, Aaron Ballman via cfe-commits < cfe-commits@lists.llvm.org> wrote: > On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith > wrote: > > rsmith added inline comments. > > > > > > Comment at: include/clang/Basic/AttrDocs.td:1628 > > @@ +1627,3 @@ > > +The ``

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith wrote: > rsmith added inline comments. > > > Comment at: include/clang/Basic/AttrDocs.td:1628 > @@ +1627,3 @@ > +The ``internal_linkage`` attribute changes the linkage type of the > declaration to internal. > +This is similar to C-sty

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

rsmith added inline comments. Comment at: include/clang/Basic/AttrDocs.td:1628 @@ +1627,3 @@ +The ``internal_linkage`` attribute changes the linkage type of the declaration to internal. +This is similar to C-style ``static``, but can be used on classes and class methods +This ca

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39128. eugenis added a comment. One more test for forward declarations. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/Se

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: include/clang/Basic/Attr.td:2125 @@ +2124,3 @@ + +def InternalLinkage : InheritableAttr { + let Spellings = [GNU<"internal_linkage">, CXX11<"clang", "internal_linkage">]; aaron.ballman wrote: > rsmith wrote: > > `Inheri

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 39124. eugenis marked 5 inline comments as done. eugenis added a comment. Disabled the new attribute on namespaces. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

aaron.ballman added a subscriber: aaron.ballman. aaron.ballman requested changes to this revision. aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. This revision now requires changes to proceed. I would like to hold off on adding the namespace attribute. There were pe

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

rsmith added inline comments. Comment at: include/clang/Basic/Attr.td:2125 @@ +2124,3 @@ + +def InternalLinkage : InheritableAttr { + let Spellings = [GNU<"internal_linkage">, CXX11<"clang", "internal_linkage">]; `InheritableAttr` doesn't seem right for the case

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

rnk accepted this revision. rnk added a reviewer: rnk. rnk added a comment. This revision is now accepted and ready to land. I'm happy with this. Richard, what do you think? Repository: rL LLVM http://reviews.llvm.org/D13925 ___ cfe-commits maili

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 38979. eugenis added a comment. Added a [[clang::internal_linkage]] spelling to the attribute. Added tests for namespace re-declarations with and without the attribute. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added a comment. In http://reviews.llvm.org/D13925#276626, @majnemer wrote: > No diagnostic is issued for the following C test case: > > int x __attribute__((internal_linkage)); > int x __attribute__((common)); > int *f() { return &x; } Thanks for noticing! Added missing attribute

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 38679. Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td include/clang/Basic/AttrDocs.td include/clang/Sema/Sema.h lib/AST/Decl.cpp lib/Sema/Sema.cpp lib/Sema/SemaDecl.cpp lib/Sema/SemaDeclAttr.cpp test/C

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

majnemer added a comment. No diagnostic is issued for the following C test case: int x __attribute__((internal_linkage)); int x __attribute__((common)); int *f() { return &x; } Comment at: include/clang/Basic/Attr.td:2112 @@ +2111,3 @@ + +def InternalLinkage : Inheritable

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:1580-1585 @@ -1577,3 +1579,8 @@ + + if (InternalLinkageAttr *Internal = D->getAttr()) { +S.Diag(Attr.getRange().getBegin(), diag::warn_attribute_ignored) +<< Attr.getName(); +S.Diag(Internal->get

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis updated this revision to Diff 38071. eugenis marked 2 inline comments as done. eugenis added a comment. This new version supports __attribute__((internal_linkage)) on classes and even namespaces! Repository: rL LLVM http://reviews.llvm.org/D13925 Files: include/clang/Basic/Attr.td

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

eugenis added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:1580-1585 @@ -1577,3 +1579,8 @@ + + if (InternalLinkageAttr *Internal = D->getAttr()) { +S.Diag(Attr.getRange().getBegin(), diag::warn_attribute_ignored) +<< Attr.getName(); +S.Diag(Internal->get

Re: [PATCH] D13925: Implement __attribute__((internal_linkage))

majnemer added a subscriber: majnemer. Comment at: include/clang/Basic/Attr.td:2114 @@ +2113,3 @@ + let Spellings = [GCC<"internal_linkage">]; + let Subjects = SubjectList<[Function,Var]>; + let Documentation = [InternalLinkageDocs]; Space between `Function` an