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

[Aaron Ballman via cfe-commits]

On Tue, Nov 3, 2015 at 7:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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).

Already planned (as a warning, because you can infer the correct
behavior anyway). Also, this can generate some documentation
automatically for those attributes.

~Aaron

>
>>
>> ~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
>
>