On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith <rich...@metafoo.co.uk> 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-style ``static``, but can be used on classes and class > methods > +This can be used to contain the ABI of a C++ library by excluding unwanted > class methods from the export tables. > ---------------- > Missing period at end of sentence. > > ================ > 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 can be used to contain the ABI of a C++ library by excluding unwanted > class methods from the export tables. > ---------------- > rsmith wrote: >> Missing period at end of sentence. > Please also say what it means if the attribute is applied to a class. > > ================ > Comment at: lib/AST/Decl.cpp:635-641 > @@ -634,2 +634,9 @@ > assert(!isa<FieldDecl>(D) && "Didn't expect a FieldDecl!"); > > + for (const DeclContext *DC = D->getDeclContext(); > + !isa<TranslationUnitDecl>(DC); DC = DC->getParent()) { > + const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC); > + if (ND && ND->getAttr<InternalLinkageAttr>()) > + return LinkageInfo::internal(); > + } > + > ---------------- > Dead code? > > ================ > Comment at: lib/AST/Decl.cpp:1362-1367 > @@ -1346,4 +1361,8 @@ > } > - assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage()); > + // Linkages may also differ if one of the declarations has > + // InternalLinkageAttr. > + assert(!Old || Old->getCachedLinkage() == D->getCachedLinkage() || > + (Old->hasAttr<InternalLinkageAttr>() != > + D->hasAttr<InternalLinkageAttr>())); > #endif > > ---------------- > We should not introduce another case where the linkage of an entity can > change after its first declaration. It seems reasonable to require this > attribute to be on the first declaration of the function. > > ================ > Comment at: lib/Sema/SemaDeclAttr.cpp:3391 > @@ +3390,3 @@ > + unsigned AttrSpellingListIndex) { > + if (checkAttrMutualExclusion<InternalLinkageAttr>(*this, D, Range, Ident)) > + return nullptr; > ---------------- > Aaron, could we move the mutual exclusion functionality to TableGen? > (Separately from this patch, of course.) It looks like there are now 6 > attributes that could use the "simple attribute" codepath if we did so.
Yes, I think that would be a good idea. Ideally, I would like the tablegen to look like: class Attr { // ... list<Attr> MutuallyExclusive = []; } def Hot { // ... let MutuallyExclusive = [Cold]; } def Cold { // ... let MutuallyExclusive = [Hot]; } I'll put this on my list of refactorings to do. ~Aaron > > > 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