[FFmpeg-devel] Visibility implementation

[Måns Rullgård]

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:
>> >> 
>> >> > On Thu, Jul 31, 2008 at 08:03:50PM +0300, Uoti Urpala wrote:
>> >> >> On Wed, 2008-07-30 at 05:45 +0300, Uoti Urpala wrote:
>> >> >> > The attached patch adds visibility information for lots of symbols in
>> >> >> 
>> >> >> I created a new version that adds visibility information for most
>> >> >> libavcodec symbols. This time I split the patch into two parts. The
>> >> >> first one has the repeated changes per internal header marking them as
>> >> >> such. The second adds visibility information for some of the symbols
>> >> >> that are not static but not declared in any header either, adds
>> >> >> exceptions for some symbols which are declared in libavcodec headers but
>> >> >> used outside it (mainly in libavformat), and defines the macros used.
>> >> >
>> >> > 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.
>
> Diego
>
> Index: libavutil/internal.h
> ===================================================================
> --- libavutil/internal.h	(revision 15672)
> +++ libavutil/internal.h	(working copy)
> @@ -50,6 +50,18 @@
>  #endif
>  #endif
>  
> +#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.  Just VISIBILITY is a bit too vague a name for the overall
control symbol, IMHO.

> +#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")))
> +#else
> +#define START_HIDDEN
> +#define END_HIDDEN
> +#define HIDDEN
> +#define VISIBLE
> +#endif
> +
>  #ifndef INT16_MIN
>  #define INT16_MIN       (-0x7fff-1)
>  #endif
> @@ -124,7 +136,7 @@
>  
>  /* math */
>  
> -extern const uint32_t ff_inverse[256];
> +extern HIDDEN const uint32_t ff_inverse[256];
>  
>  #if defined(ARCH_X86)
>  #    define FASTDIV(a,b) \
> Index: libavcodec/mjpeg.h
> ===================================================================
> --- 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?

-- 
M?ns Rullg?rd
mans at mansr.com