[Xfce4-commits] <xfce4-dev-tools:master> Don't set the default visibility in XDT_FEATURE_VISIBILITY.

[Jannis Pohlmann]

On Thu, 22 Oct 2009 14:16:19 -0700
"Brian J. Tarricone" <brian at tarricone.org> wrote:

> On Thu, Oct 22, 2009 at 04:50, Nick Schermer <nickschermer at gmail.com>
> wrote:
> > 2009/10/22 Nick Schermer <nickschermer at gmail.com>:
> >> 2009/10/22 Brian J. Tarricone <brian at tarricone.org>:
> >>> Nick, revert this please.  If you don't want to use visibility
> >>> properly in your module, then don't use the macro.  Don't make it
> >>> useless for everyone else.
> >>
> >> Define useless. You think this changes anything in, for example,
> >> xfconf?
> >
> > Let's put in another way, it is better to set the default visibility
> > level for the library you compile if you want to use this. The
> > global visibility makes this unusable for thunar (break modules),
> > exo (gio module and private hal library), 4ui (private kbd lib) and
> > the panel (all plugins). So that leaves use with xfconf and 4util.
> 
> By using that autoconf macro you are saying "I want to use the
> compiler's visibility features to only export public API from the
> library."  Obviously, if you are going to do this, you must define
> what's public.  If you are not going to take the time to define what's
> public, then you should not use this macro.
> 
> Furthermore, with your change, there is no straightforward way of
> setting the default visibility to hidden if that's what you want.
> Well, ok, you can check the value of $have_gnuc_visibility in
> configure.ac, but that's retarded.
> 
> I would consider (one of) a couple changes from my original macro:
> 
> 1.  Allowing the macro to take an argument specifying the default
> visibility (defaults to "hidden" if not specified).
> 2.  Allowing the macro to take an argument specifying a variable to
> stuff the visibility argument in, so you can add it (or not add it) to
> different targets in the module (assuming there's more than one).  If
> not specified, it defaults to putting the vis parameter in CFLAGS.

I'm thinking back and forth here... Nick's concern about
-fvisibility=hidden is that exo needs HAVE_GNUC_VISIBILITY for the
alias generation but exo's .symbols file has a few exceptions (e.g.
exo_atomic_{inc,dec}) which are not declared visible when
-fvisiblity=hidden is passed to the compiler. In consequence they don't
show up in the .so file even though they would (without -fvisib...)
because they'd not be hidden and would pass -export-regex-symbols as
well. So there are expections where the alias stuff is used but
-fvisibility=hidden is not desired. 

Soo, for the entire afternoon I was thinking, yeah, let's drop
-fvisibility=hidden from XDT_FEATURE_VISIBILITY(). However, without it
XDT_FEATURE_VISIBILITY() doesn't do much anymore. I think it's all
about how it is interpreted. If the entire point of
XDT_FEATURE_VISIBILITY() is seen in hiding all symbols if possible,
then yes, -fvisibility=hidden has to go back in. 

I can also see how we could make it useful and more flexible by adding
a parameter to it (e.g. a compiler flag variable), like you suggested
on bugzilla.xfce.org. For aliasing, libraries need HAVE_GNUC_VISIBILITY
but they may or may not want -fvisibility=hidden.

> Regardless: revert your change.  And in the future, at least do me the
> common courtesy of discussing changes to my code in a module I
> maintain before checking things in.  Jannis convinced me we can all
> behave properly without having tighter commit permissions on git;
> please don't prove him wrong.

Yes, I think I agree. Revert for now, especially since we're not using
XDT_FEATURE_VISIBILITY() in exo anymore after the removal of the alias
stuff has been reverted as well.

  - Jannis
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://mail.xfce.org/pipermail/xfce4-dev/attachments/20091023/0e0d121e/attachment.pgp>