On Tue, Nov 3, 2015 at 7:23 PM, Richard Smith <[email protected]> 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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
