[Xcb] hidden-visibility and library size

[Vincent Torri]

On Tue, 27 Dec 2005, Jamey Sharp wrote:
>
> I think setting visibility hidden by default is the wrong choice here.
> Instead of setting any compile flags or touching any public header
> files, just fix xcbint.h so that everything there is marked either
> hidden or internal.

right

you can find some doc here : http://gcc.gnu.org/wiki/Visibility

especially the new pragma in gcc 4.0. It can be helpfull, i think.

> > my point of view is : when i debug my code, i don't want to debug the
> > compiler optimizations. that's why I prefer to use only -g.
>
> That's fine, but it's *your* preference: don't impose it on me. :-)

hehe, i impose nothing :) I just present my point of view :)

> Here are some comments on your changes. I'd strongly prefer a single
> patch in the form produced by `cvs diff -Nu`, by the way.
>
> > --- Makefile.am.~1.26.~	2005-12-24 14:59:59.000000000 +0100
> > +++ Makefile.am	2005-12-27 22:09:59.053154976 +0100
> > @@ -41,8 +41,9 @@
> >  xcbinclude_HEADERS = xcb.h xcbext.h xcbxlib.h $(COREHEADERS) $(EXTHEADERS)
> >
> >  CFLAGS =
> > -AM_CFLAGS = -include config.h $(CDEBUGFLAGS) $(XCBPROTO_CFLAGS) $(XPROTO_CFLAGS) $(XAU_CFLAGS)
> > +AM_CFLAGS = -include config.h $(CDEBUGFLAGS) $(VISIBILITY_CFLAGS) $(XCBPROTO_CFLAGS) $(XPROTO_CFLAGS) $(XAU_CFLAGS)
> >  libXCB_la_LIBADD = $(XCBPROTO_LIBS) $(XPROTO_LIBS) $(XAU_LIBS)
> > +libXCB_la_LDFLAGS = -s
>
> You've included your strip change in this patch; it should be cleaned up
> before commit.

right. I've forgotten to remove it.

> > dnl Detection and configuration of the visibility feature of gcc
> > dnl Vincent Torri 2005-12-24
> > dnl
> > dnl AC_CHECK_VISIBILITY([ACTION-IF-FOUND [, ACTION-IF-NOT-FOUND]]])
> > dnl Check the visibility feature of gcc and define VISIBILITY_CFLAGS
> > dnl
> > AC_DEFUN([AC_CHECK_VISIBILITY],
> >    [VISIBILITY_CFLAGS=""
> >     AC_MSG_CHECKING([whether ${CC} supports -fvisibility=hidden])
> >     save_CFLAGS=${CFLAGS}
> >     CFLAGS="$CFLAGS -fvisibility=hidden"
> >     AC_TRY_LINK(
> >        [],
> >        [return 0;],
> >        [VISIBILITY_CFLAGS="-fvisibility=hidden -DHAVE_VISIBILITY_FEATURE"
> >         m4_if([$1], [], [:], [$1])],
> >        [m4_if([$2], [], [:], [$2])])
> >     AC_MSG_RESULT(${visibility})
> >     CFLAGS=${save_CFLAGS}
> >     AC_SUBST(VISIBILITY_CFLAGS)
> >    ])
>
> Maybe the macro should try compiling a function with the visibility
> attribute instead of testing for -fvisibility?

if we don't set all the exported symbols as hidden, then yes.

> It should probably use AC_COMPILE_IFELSE rather than AC_TRY_LINK.

AC_TRY_COMPILE should be sufficient, i think.

> I'd rather use
> 	AC_DEFINE(HAVE_VISIBILITY_FEATURE)
> instead of passing a -D flag to the compiler.

indeed. It's better :) It simplifies also the modifications.

>
> > --- configure.ac	2005-12-27 22:53:31.313031112 +0100
> > +++ configure.ac.new	2005-12-27 22:53:14.602571488 +0100
> > @@ -5,7 +5,9 @@
> >  AC_INIT([libXCB],
> >          0.9,
> >          [xcb at lists.freedesktop.org])
> > +
> >  AC_CONFIG_SRCDIR([xcb.pc.in])
> > +
>
> Please don't include whitespace patches like this.

ok. I find the code more readable with them, but, i'll remove them

> > @@ -78,4 +80,29 @@
> >  	-Wstrict-prototypes -Wmissing-declarations -Wnested-externs"
> >  AC_SUBST(CDEBUGFLAGS)
> >
> > -AC_OUTPUT([Makefile src/Makefile tests/Makefile xcb.pc])
> > +AC_CHECK_VISIBILITY([has_visibility="yes"], [has_visibility="no"])
> > +
> > +AC_CONFIG_FILES([Makefile src/Makefile tests/Makefile])
> > +AC_CONFIG_FILES([xcb.pc])
> > +
> > +AC_OUTPUT
>
> I don't understand. Why did you change the AC_OUTPUT invocation to use
> AC_CONFIG_FILES? If it's important, please post a separate patch for it.

it's the recommended way to do that (according to the autoconf doc). See
obsolete construct, section 15.

> > +
> > +#####################################################################
> > +## Info
> > +
> > +echo
> > +echo "------------------------------------------------------------------------"
> > +echo " $PACKAGE $VERSION"
> > +echo "------------------------------------------------------------------------"
> > +echo
> > +echo " Visibility feature..: $has_visibility"
> > +echo " Optimisation flags..: $optflags"
> > +echo
> > +echo " Install path........: $prefix"
> > +echo "   binaries..........: $bindir"
> > +echo "   libraries.........: ${libdir}"
> > +echo "   headers...........: ${xcbincludedir}"
> > +echo
> > +echo " Compilation.........: make"
> > +echo " Installation........: make install"
> > +echo
>
> Obviously this change shouldn't be part of this patch. I'm not sure I
> want it committed at all, though I don't feel strongly about that.

it can tell the user which flags he is using, where the library will be
installed and what to do. I find these informations usefull. Especially
when i want to compile a lib with specific flags, to be sure i've not
forgotten one. It's always my point of view, I impose nothing, i propose
something ;)

Vincent