<div class="gmail_quote">On Sun, Jan 1, 2012 at 5:53 PM, Rafael Ávila de Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">> Anyway, I'm not actually convinced there's a bug here: unless I'm<br>
> mistaken, there isn't any legal way to refer to a type with hidden<br>
> visibility outside the shared library where it is defined.  Or am I<br>
> missing something?<br>
<br>
</div>If the "struct zed" definition is on a header, it is should be possible<br>
to include that header when building a library with -fvisibility=hidden<br>
and use the same header in a program that uses that library.<br>
<div class="im"><br>
> This falls out of my question as well -- why doesn't merge DTRT here?<br>
> Put another way, once we encounter an explicit visibility specification<br>
> for a declaration, why would we ever merge it away? (This is distinct<br>
> from computing a visibility for a declaration from that of a set of N<br>
> other declarations, some of which use an explicit visibility spec, some<br>
> of which do not.) I feel like we should have better logic to make an<br>
> explicit visibility spec trump all else while still merging linkage....<br>
> The current logic for merging of explicitly specified visibility makes<br>
> no sense to me.<br>
<br>
</div>I tested this and it works. It also found another bug in the way we<br>
merge visibilities. An update patch is attached.</blockquote><div><br></div><div>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.</div>
<br>A few comments on the patch directly:<br><br>diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h<br>index 52e09e6..ede31e2 100644<br>--- a/include/clang/AST/Decl.h<br>+++ b/include/clang/AST/Decl.h<br>@@ -252,15 +252,17 @@ public:<br>
       setLinkage(minLinkage(linkage(), L));<br>     }<br>     void mergeLinkage(LinkageInfo Other) {<br>-      setLinkage(minLinkage(linkage(), Other.linkage()));<br>+      mergeLinkage(Other.linkage());<br>     }<br> <br>
-    void mergeVisibility(Visibility V) {<br>-      setVisibility(minVisibility(visibility(), V));<br>-    }<br>     void mergeVisibility(Visibility V, bool E) {<br>+      if (visibilityExplicit())<br>+        return;<br>
       setVisibility(minVisibility(visibility(), V), visibilityExplicit() || E);<br>}<br>+ void mergeVisibility(Visibility V) {<br>+ mergeVisibility(V, visibilityExplicit());<br><br>This is the same as 'mergeVisibility(V, false)', why not just make E have a default argument of false?<br>
<br>diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp<br>index b4a4eb1..30669b1 100644<br>--- a/lib/AST/Decl.cpp<br>+++ b/lib/AST/Decl.cpp<br>@@ -340,8 +340,7 @@ static LinkageInfo getLVForNamespaceScopeDecl(const NamedDecl *D, LVFlags F) {<br>
       LVPair TypeLV = Var->getType()->getLinkageAndVisibility();<br>       if (TypeLV.first != ExternalLinkage)<br>         return LinkageInfo::uniqueExternal();<br>-      if (!LV.visibilityExplicit())<br>-        LV.mergeVisibility(TypeLV.second);<br>
+      LV.mergeVisibility(TypeLV.second);<br>     }<br> <br>     if (Var->getStorageClass() == SC_PrivateExtern)<br>@@ -602,12 +601,9 @@ static LinkageInfo getLVForClassMember(const NamedDecl *D, LVFlags F) {<br>     LVPair TypeLV = VD->getType()->getLinkageAndVisibility();<br>
     if (TypeLV.first != ExternalLinkage)<br>       LV.mergeLinkage(UniqueExternalLinkage);<br>-    if (!LV.visibilityExplicit())<br>-      LV.mergeVisibility(TypeLV.second);<br>+    LV.mergeVisibility(TypeLV.second);<br>
   }<br> <br>-  F.ConsiderGlobalVisibility &= !LV.visibilityExplicit();<br><br>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?</div>