[FFmpeg-devel] [PATCHv2] all: fix enum definition for large values

[Ganesh Ajjanagadde]

On Wed, Oct 28, 2015 at 7:00 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Tue, Oct 27, 2015 at 10:58 PM, Ganesh Ajjanagadde
> <gajjanagadde at gmail.com> wrote:
>>
>> On Tue, Oct 27, 2015 at 10:41 PM, Ronald S. Bultje <rsbultje at gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Tue, Oct 27, 2015 at 8:53 PM, Ganesh Ajjanagadde
>> > <gajjanagadde at gmail.com>
>> > wrote:
>> >>
>> >> ISO C restricts enumerator values to the range of int. Thus (for
>> >> instance)
>> >> 0x80000000
>> >> unfortunately does not work, and throws a warning with -Wpedantic on
>> >> clang 3.7.
>> >>
>> >> This fixes it by using alternative expressions that result in identical
>> >> values but do not have this issue.
>> >>
>> >> Tested with FATE.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> ---
>> >>  libavcodec/dca_syncwords.h | 26 ++++++++++++--------------
>> >>  libavformat/cinedec.c      |  2 +-
>> >>  libavformat/mov_chan.c     |  2 +-
>> >>  3 files changed, 14 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/libavcodec/dca_syncwords.h b/libavcodec/dca_syncwords.h
>> >> index 3466b6b..6981cb8 100644
>> >> --- a/libavcodec/dca_syncwords.h
>> >> +++ b/libavcodec/dca_syncwords.h
>> >> @@ -19,19 +19,17 @@
>> >>  #ifndef AVCODEC_DCA_SYNCWORDS_H
>> >>  #define AVCODEC_DCA_SYNCWORDS_H
>> >>
>> >> -enum DCASyncwords {
>> >> -    DCA_SYNCWORD_CORE_BE        = 0x7FFE8001U,
>> >> -    DCA_SYNCWORD_CORE_LE        = 0xFE7F0180U,
>> >> -    DCA_SYNCWORD_CORE_14B_BE    = 0x1FFFE800U,
>> >> -    DCA_SYNCWORD_CORE_14B_LE    = 0xFF1F00E8U,
>> >> -    DCA_SYNCWORD_XCH            = 0x5A5A5A5AU,
>> >> -    DCA_SYNCWORD_XXCH           = 0x47004A03U,
>> >> -    DCA_SYNCWORD_X96            = 0x1D95F262U,
>> >> -    DCA_SYNCWORD_XBR            = 0x655E315EU,
>> >> -    DCA_SYNCWORD_LBR            = 0x0A801921U,
>> >> -    DCA_SYNCWORD_XLL            = 0x41A29547U,
>> >> -    DCA_SYNCWORD_SUBSTREAM      = 0x64582025U,
>> >> -    DCA_SYNCWORD_SUBSTREAM_CORE = 0x02B09261U,
>> >> -};
>> >> +#define    DCA_SYNCWORD_CORE_BE              0x7FFE8001U
>> >> +#define    DCA_SYNCWORD_CORE_LE              0xFE7F0180U
>> >> +#define    DCA_SYNCWORD_CORE_14B_BE          0x1FFFE800U
>> >> +#define    DCA_SYNCWORD_CORE_14B_LE          0xFF1F00E8U
>> >> +#define    DCA_SYNCWORD_XCH                  0x5A5A5A5AU
>> >> +#define    DCA_SYNCWORD_XXCH                 0x47004A03U
>> >> +#define    DCA_SYNCWORD_X96                  0x1D95F262U
>> >> +#define    DCA_SYNCWORD_XBR                  0x655E315EU
>> >> +#define    DCA_SYNCWORD_LBR                  0x0A801921U
>> >> +#define    DCA_SYNCWORD_XLL                  0x41A29547U
>> >> +#define    DCA_SYNCWORD_SUBSTREAM            0x64582025U
>> >> +#define    DCA_SYNCWORD_SUBSTREAM_CORE       0x02B09261U
>> >
>> >
>> > This one is fine.
>> >
>> >>
>> >> --- a/libavformat/cinedec.c
>> >> +++ b/libavformat/cinedec.c
>> >> @@ -50,7 +50,7 @@ enum {
>> >>      CFA_BAYER     = 3,  /**< GB/RG */
>> >>      CFA_BAYERFLIP = 4,  /**< RG/GB */
>> >>
>> >> -    CFA_TLGRAY    = 0x80000000,
>> >> +    CFA_TLGRAY    = INT32_MIN,
>> >>      CFA_TRGRAY    = 0x40000000,
>> >>      CFA_BLGRAY    = 0x20000000,
>> >>      CFA_BRGRAY    = 0x10000000
>> >> diff --git a/libavformat/mov_chan.c b/libavformat/mov_chan.c
>> >> index a2fa8d6..f6181e2 100644
>> >> --- a/libavformat/mov_chan.c
>> >> +++ b/libavformat/mov_chan.c
>> >> @@ -45,7 +45,7 @@
>> >>   *            do not specify a particular ordering of those channels."
>> >>   */
>> >>  enum MovChannelLayoutTag {
>> >> -    MOV_CH_LAYOUT_UNKNOWN               = 0xFFFF0000,
>> >> +    MOV_CH_LAYOUT_UNKNOWN               = -( 1 << 16),
>> >>      MOV_CH_LAYOUT_USE_DESCRIPTIONS      = (  0 << 16) | 0,
>> >>      MOV_CH_LAYOUT_USE_BITMAP            = (  1 << 16) | 0,
>> >>      MOV_CH_LAYOUT_DISCRETEINORDER       = (147 << 16) | 0,
>> >> --
>> >> 2.6.2
>> >
>> >
>> > I personally don't really like these... I think both obfuscate the
>> > meaning
>> > of the flag values, particularly the first one.
>>
>> There is no real solution (recall apedec and the INT32_MIN final
>> solution), barring adding a comment signifying our intent (ie the
>> desired hex mask). I can do this if you think it helps.
>
>
> The solution is to not care about ISO C if it doesn't fix real issues. :)

This is where we will just have to agree to disagree, I consider this
issue "real enough" - it is a violation of the standard, and POSIX
says nothing contrariwise unlike the function pointer/data pointer
thing.

>
> Ronald