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

[Richard Smith via cfe-commits]

On Tue, Nov 3, 2015 at 4:37 PM, Aaron Ballman via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith <richard at 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.
>

It'd be nice if we could also diagnose inconsistencies in this (eg, Hot
says it's exclusive with Cold and Cold doesn't agree).


> ~Aaron
>
> >
> >
> > Repository:
> >   rL LLVM
> >
> > http://reviews.llvm.org/D13925
> >
> >
> >
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20151103/0c7af2a3/attachment-0001.html>