[FFmpeg-devel] Visibility implementation

[Diego Biurrun]

On Thu, Oct 23, 2008 at 09:25:35PM +0100, M?ns Rullg?rd wrote:
> Diego Biurrun <diego at biurrun.de> writes:
> 
> > On Sun, Oct 19, 2008 at 01:48:10PM +0100, M?ns Rullg?rd wrote:
> >> Diego Biurrun <diego at biurrun.de> writes:
> >> 
> >> > On Sat, Oct 18, 2008 at 11:57:17AM +0100, M?ns Rullg?rd wrote:
> >> >> Diego Biurrun <diego at biurrun.de> writes:
> >> >> >
> >> >> > Here is a new version of those patches, updated for HEAD.  Uoti, maybe
> >> >> > you can doublecheck nothing is missing...
> >> >> >
> >> >> > --- a/libavutil/internal.h
> >> >> > +++ b/libavutil/internal.h
> >> >> > @@ -50,6 +50,12 @@
> >> >> >  
> >> >> > +#define START_HIDDEN_VISIBILITY_SECTION _Pragma("GCC visibility push(hidden)")
> >> >> > +#define END_VISIBILITY_SECTION          _Pragma("GCC visibility pop")
> >> >> > +
> >> >> > +#define HIDDEN             __attribute__((visibility("hidden")))
> >> >> > +#define EXTERNALLY_VISIBLE __attribute__((visibility("default")))
> >> >> 
> >> >> These should be under appropriate ifdefs.  The names are awfully long
> >> >> too.
> >> >
> >> > START_HIDDEN_VISIBILITY
> >> > END_HIDDEN_VISIBILITY
> >> 
> >> What about this:
> >> 
> >> +#define START_HIDDEN _Pragma("GCC visibility push(hidden)")
> >> +#define END_HIDDEN   _Pragma("GCC visibility pop")
> >> +
> >> +#define HIDDEN  __attribute__((visibility("hidden")))
> >> +#define VISIBLE __attribute__((visibility("default")))
> >> 
> >> Just because a feature was designed to work around C++ bloat, we don't
> >> have to adopt their bloated naming style when we use it.
> >
> > Updated version attached.
> >
> > --- libavutil/internal.h	(revision 15672)
> > +++ libavutil/internal.h	(working copy)
> > @@ -50,6 +50,18 @@
> >  
> > +#ifdef CONFIG_VISIBILITY
> 
> Where do you expect this to get set?  Is there any reason to disable
> it when the compiler supports it?  I would expect to see some compiler
> version ifdeffery or a configure check setting HAVE_ATTR_VISIBILITY or
> similar.

ATM it would have to be set manually in config.h, i.e. configure support
is still missing.  I'll see if I implement it in the next round.

> Just VISIBILITY is a bit too vague a name for the overall
> control symbol, IMHO.

OK, I will change that.

> > --- libavcodec/mjpeg.h	(revision 15672)
> > +++ libavcodec/mjpeg.h	(working copy)
> > @@ -37,6 +37,9 @@
> >  #include "bitstream.h"
> >  
> >  
> > +START_HIDDEN
> > +
> > +
> >  /* JPEG marker codes */
> >  typedef enum {
> >      /* start of frame */
> > @@ -152,4 +155,7 @@
> >                                    const uint8_t *bits_table,
> >                                    const uint8_t *val_table);
> >  
> > +
> > +END_HIDDEN
> > +
> >  #endif /* AVCODEC_MJPEG_H */
> 
> Why so many blank lines?

Why not?

Diego