[cfe-commits] [PR10113][patch] Don't consider template arguments when the visibility is already explicit

[Chandler Carruth]

On Sun, Jan 1, 2012 at 5:53 PM, Rafael Ávila de Espíndola <
rafael.espindola at gmail.com> wrote:

> > Anyway, I'm not actually convinced there's a bug here: unless I'm
> > mistaken, there isn't any legal way to refer to a type with hidden
> > visibility outside the shared library where it is defined.  Or am I
> > missing something?
>
> If the "struct zed" definition is on a header, it is should be possible
> to include that header when building a library with -fvisibility=hidden
> and use the same header in a program that uses that library.
>
> > This falls out of my question as well -- why doesn't merge DTRT here?
> > Put another way, once we encounter an explicit visibility specification
> > for a declaration, why would we ever merge it away? (This is distinct
> > from computing a visibility for a declaration from that of a set of N
> > other declarations, some of which use an explicit visibility spec, some
> > of which do not.) I feel like we should have better logic to make an
> > explicit visibility spec trump all else while still merging linkage....
> > The current logic for merging of explicitly specified visibility makes
> > no sense to me.
>
> I tested this and it works. It also found another bug in the way we
> merge visibilities. An update patch is attached.


In general, this makes a bit more sense to me. I'd still like Eli to check
that we're doing the right thing w/ visibility though, I'm not really
confident in my understanding of the desired results for all of these cases.

A few comments on the patch directly:

diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h
index 52e09e6..ede31e2 100644
--- a/include/clang/AST/Decl.h
+++ b/include/clang/AST/Decl.h
@@ -252,15 +252,17 @@ public:
setLinkage(minLinkage(linkage(), L));
}
void mergeLinkage(LinkageInfo Other) {
- setLinkage(minLinkage(linkage(), Other.linkage()));
+ mergeLinkage(Other.linkage());
}

- void mergeVisibility(Visibility V) {
- setVisibility(minVisibility(visibility(), V));
- }
void mergeVisibility(Visibility V, bool E) {
+ if (visibilityExplicit())
+ return;
setVisibility(minVisibility(visibility(), V), visibilityExplicit() || E);
}
+ void mergeVisibility(Visibility V) {
+ mergeVisibility(V, visibilityExplicit());

This is the same as 'mergeVisibility(V, false)', why not just make E have a
default argument of false?

diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index b4a4eb1..30669b1 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -340,8 +340,7 @@ static LinkageInfo getLVForNamespaceScopeDecl(const
NamedDecl *D, LVFlags F) {
LVPair TypeLV = Var->getType()->getLinkageAndVisibility();
if (TypeLV.first != ExternalLinkage)
return LinkageInfo::uniqueExternal();
- if (!LV.visibilityExplicit())
- LV.mergeVisibility(TypeLV.second);
+ LV.mergeVisibility(TypeLV.second);
}

if (Var->getStorageClass() == SC_PrivateExtern)
@@ -602,12 +601,9 @@ static LinkageInfo getLVForClassMember(const NamedDecl
*D, LVFlags F) {
LVPair TypeLV = VD->getType()->getLinkageAndVisibility();
if (TypeLV.first != ExternalLinkage)
LV.mergeLinkage(UniqueExternalLinkage);
- if (!LV.visibilityExplicit())
- LV.mergeVisibility(TypeLV.second);
+ LV.mergeVisibility(TypeLV.second);
}

- F.ConsiderGlobalVisibility &= !LV.visibilityExplicit();

I don't entirely understand this change, but I'm not deeply familiar with
the code. I'd just like someone who is to ensure this is correct. I assume
this is tested by your changes to the codegen.cpp test?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120101/162b1e75/attachment.html>