[FFmpeg-devel] Visibility implementation
Måns Rullgård
mans
Thu Oct 23 22:25:35 CEST 2008
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
More information about the ffmpeg-devel
mailing list