[Xcb] XCB is ANSI C?

[Barton C Massey]

I submitted some changes recently to make XCB's demos strict
ANSI C again---it was essentially free.

My opinion is that XCB itself should also always remain
compilable by a strict ANSI C compiler---not every place we
want to field this thing is going to have a GCC port.

I mention this only because I'm not sure what effect the
suggested changes to xlibint.h would have---probably none if
the ifdefing is done right.

Is there anyone who has a disagreement with XCB and its
shipped clients being pure standards-compliant ANSI C?
Speak now if so...

	Bart

In message <20051228005101.GD8079 at id.minilop.net> Jamey wrote:
> 
> On Wed, Dec 28, 2005 at 12:14:46AM +0100, Vincent Torri wrote:
> > Of course, we need to specify all the exported functions. There's an
> > example in xcb.h.diff.
> 
> Oh, so that's how the visibility stuff works. OK. Thanks for putting
> this together!
> 
> 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. For XCB, internal should always be safe (we never
> expose to external code pointers to internal functions) and buys us some
> more speed and code size improvements.
> 
> > I've once tried to debug with -g -O2 a lib (evas, iirc) and gdb has
> > given me odd values when i used bt f
> 
> Yes, that's normal. Debugging can be hard when combined with
> optimization. But it's not impossible, and I do it on a regular basis,
> especially with XCB where the optimizer doesn't seem to do much.
> 
> > 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. :-)
> 
> 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 =3D xcb.h xcbext.h xcbxlib.h $(COREHEADERS) $(EXTHEAD=
> ERS)
> > =20
> >  CFLAGS =3D
> > -AM_CFLAGS =3D -include config.h $(CDEBUGFLAGS) $(XCBPROTO_CFLAGS) $(XPRO=
> TO_CFLAGS) $(XAU_CFLAGS)
> > +AM_CFLAGS =3D -include config.h $(CDEBUGFLAGS) $(VISIBILITY_CFLAGS) $(XC=
> BPROTO_CFLAGS) $(XPROTO_CFLAGS) $(XAU_CFLAGS)
> >  libXCB_la_LIBADD =3D $(XCBPROTO_LIBS) $(XPROTO_LIBS) $(XAU_LIBS)
> > +libXCB_la_LDFLAGS =3D -s
> 
> You've included your strip change in this patch; it should be cleaned up
> before commit.
> 
> > 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=3D""
> >     AC_MSG_CHECKING([whether ${CC} supports -fvisibility=3Dhidden])
> >     save_CFLAGS=3D${CFLAGS}
> >     CFLAGS=3D"$CFLAGS -fvisibility=3Dhidden"
> >     AC_TRY_LINK(
> >        [],
> >        [return 0;],
> >        [VISIBILITY_CFLAGS=3D"-fvisibility=3Dhidden -DHAVE_VISIBILITY_FEAT=
> URE"
> >         m4_if([$1], [], [:], [$1])],
> >        [m4_if([$2], [], [:], [$2])])
> >     AC_MSG_RESULT(${visibility})
> >     CFLAGS=3D${save_CFLAGS}
> >     AC_SUBST(VISIBILITY_CFLAGS)
> >    ])
> 
> Maybe the macro should try compiling a function with the visibility
> attribute instead of testing for -fvisibility?
> 
> It should probably use AC_COMPILE_IFELSE rather than AC_TRY_LINK.
> 
> I'd rather use
> 	AC_DEFINE(HAVE_VISIBILITY_FEATURE)
> instead of passing a -D flag to the compiler.
> 
> > --- 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.
> 
> > @@ -78,4 +80,29 @@
> >  	-Wstrict-prototypes -Wmissing-declarations -Wnested-externs"
> >  AC_SUBST(CDEBUGFLAGS)
> > =20
> > -AC_OUTPUT([Makefile src/Makefile tests/Makefile xcb.pc])
> > +AC_CHECK_VISIBILITY([has_visibility=3D"yes"], [has_visibility=3D"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.
> 
> > +
> > +#####################################################################
> > +## 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.
> 
> --Jamey
>