diff mbox series

[FFmpeg-devel] avcodec/cbs: Avoid leaving the ... out in calls to variadic macros

Message ID 20200323005220.16823-1-andreas.rheinhardt@gmail.com
State Accepted
Commit 14dd0a9057019e97ff9438f6cc1502f6922acb85
Headers show
Series [FFmpeg-devel] avcodec/cbs: Avoid leaving the ... out in calls to variadic macros
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt March 23, 2020, 12:52 a.m. UTC
According to C99, there has to be at least one argument for every ...
in a variadic function-like macro. In practice most (all?) compilers also
allow to leave it completely out, but it is nevertheless required: In a
variadic macro "there shall be more arguments in the invocation than there
are parameters in the macro definition (excluding the ...)." (C99,
6.10.3.4).

CBS (not the framework itself, but the macros used in the
cbs_*_syntax_template.c files) relies on the compiler allowing to leave
a variadic macro argument out. This leads to warnings when compiling in
-pedantic mode, e.g. "warning: must specify at least one argument for
'...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
from Clang.

Most of these warnings can be easily avoided: The syntax_templates
mostly contain helper macros that expand to more complex variadic macros
and these helper macros often omit an argument for the .... Modifying
them to always expand to complex macros with an empty argument for the
... at the end fixes most of these warnings: The number of warnings went
down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.

These eight remaining warnings for cbs_vp9 have been fixed by switching
to another macro in cbs_vp9_syntax_template: The fixed values for the
sync bytes as well as the trailing bits for byte-alignment are now read
via the fixed() macro (this also adds a check to ensure that trailing
bits are indeed zero as they have to be).

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
There are two ways to fix the remaining 32 warnings from cbs_h2645:

Simply add ", " to all macro calls that make use of the complex macros;
this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.

Or add new macros for these macro calls: The places that produce
warnings use the complex macros directly, because they use names
different from the default names that the helper macros use, but they do
not use subscripts and therefore leave the variadic argument (designed
for subscripts) out. I would have implemented the second solution if it
were not for the problem of the naming of the new macros.

(There is of course also the possibility not to care about the remaining
ones.)

 libavcodec/cbs_av1.c                 | 16 ++++++++--------
 libavcodec/cbs_h2645.c               | 14 +++++++-------
 libavcodec/cbs_jpeg.c                |  2 +-
 libavcodec/cbs_mpeg2.c               |  6 +++---
 libavcodec/cbs_vp9.c                 | 13 ++++++-------
 libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
 6 files changed, 29 insertions(+), 43 deletions(-)

Comments

Andreas Rheinhardt March 31, 2020, 1:16 p.m. UTC | #1
Andreas Rheinhardt:
> According to C99, there has to be at least one argument for every ...
> in a variadic function-like macro. In practice most (all?) compilers also
> allow to leave it completely out, but it is nevertheless required: In a
> variadic macro "there shall be more arguments in the invocation than there
> are parameters in the macro definition (excluding the ...)." (C99,
> 6.10.3.4).
> 
> CBS (not the framework itself, but the macros used in the
> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
> a variadic macro argument out. This leads to warnings when compiling in
> -pedantic mode, e.g. "warning: must specify at least one argument for
> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
> from Clang.
> 
> Most of these warnings can be easily avoided: The syntax_templates
> mostly contain helper macros that expand to more complex variadic macros
> and these helper macros often omit an argument for the .... Modifying
> them to always expand to complex macros with an empty argument for the
> ... at the end fixes most of these warnings: The number of warnings went
> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
> 
> These eight remaining warnings for cbs_vp9 have been fixed by switching
> to another macro in cbs_vp9_syntax_template: The fixed values for the
> sync bytes as well as the trailing bits for byte-alignment are now read
> via the fixed() macro (this also adds a check to ensure that trailing
> bits are indeed zero as they have to be).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> There are two ways to fix the remaining 32 warnings from cbs_h2645:
> 
> Simply add ", " to all macro calls that make use of the complex macros;
> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
> 
> Or add new macros for these macro calls: The places that produce
> warnings use the complex macros directly, because they use names
> different from the default names that the helper macros use, but they do
> not use subscripts and therefore leave the variadic argument (designed
> for subscripts) out. I would have implemented the second solution if it
> were not for the problem of the naming of the new macros.
> 
> (There is of course also the possibility not to care about the remaining
> ones.)
> 
>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>  libavcodec/cbs_jpeg.c                |  2 +-
>  libavcodec/cbs_mpeg2.c               |  6 +++---
>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>  6 files changed, 29 insertions(+), 43 deletions(-)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index 16eb7143b6..fc228086c2 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -550,12 +550,12 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define fb(width, name) \
> -        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
> +        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>  #define fc(width, name, range_min, range_max) \
> -        xf(width, name, current->name, range_min, range_max, 0)
> +        xf(width, name, current->name, range_min, range_max, 0, )
>  #define flag(name) fb(1, name)
>  #define su(width, name) \
> -        xsu(width, name, current->name, 0)
> +        xsu(width, name, current->name, 0, )
>  
>  #define fbs(width, name, subs, ...) \
>          xf(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
> @@ -568,7 +568,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>  
>  #define fixed(width, name, value) do { \
>          av_unused uint32_t fixed_value = value; \
> -        xf(width, name, fixed_value, value, value, 0); \
> +        xf(width, name, fixed_value, value, value, 0, ); \
>      } while (0)
>  
>  
> @@ -623,9 +623,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>  #define delta_q(name) do { \
>          uint8_t delta_coded; \
>          int8_t delta_q; \
> -        xf(1, name.delta_coded, delta_coded, 0, 1, 0); \
> +        xf(1, name.delta_coded, delta_coded, 0, 1, 0, ); \
>          if (delta_coded) \
> -            xsu(1 + 6, name.delta_q, delta_q, 0); \
> +            xsu(1 + 6, name.delta_q, delta_q, 0, ); \
>          else \
>              delta_q = 0; \
>          current->name = delta_q; \
> @@ -700,9 +700,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>      } while (0)
>  
>  #define delta_q(name) do { \
> -        xf(1, name.delta_coded, current->name != 0, 0, 1, 0); \
> +        xf(1, name.delta_coded, current->name != 0, 0, 1, 0, ); \
>          if (current->name) \
> -            xsu(1 + 6, name.delta_q, current->name, 0); \
> +            xsu(1 + 6, name.delta_q, current->name, 0, ); \
>      } while (0)
>  
>  #define leb128(name) do { \
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 7a4eecf439..d42073cc5a 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -250,18 +250,18 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define u(width, name, range_min, range_max) \
> -        xu(width, name, current->name, range_min, range_max, 0)
> +        xu(width, name, current->name, range_min, range_max, 0, )
>  #define ub(width, name) \
> -        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
> +        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>  #define flag(name) ub(1, name)
>  #define ue(name, range_min, range_max) \
> -        xue(name, current->name, range_min, range_max, 0)
> +        xue(name, current->name, range_min, range_max, 0, )
>  #define i(width, name, range_min, range_max) \
> -        xi(width, name, current->name, range_min, range_max, 0)
> +        xi(width, name, current->name, range_min, range_max, 0, )
>  #define ib(width, name) \
> -        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0)
> +        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0, )
>  #define se(name, range_min, range_max) \
> -        xse(name, current->name, range_min, range_max, 0)
> +        xse(name, current->name, range_min, range_max, 0, )
>  
>  #define us(width, name, range_min, range_max, subs, ...) \
>          xu(width, name, current->name, range_min, range_max, subs, __VA_ARGS__)
> @@ -280,7 +280,7 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  
>  #define fixed(width, name, value) do { \
>          av_unused uint32_t fixed_value = value; \
> -        xu(width, name, fixed_value, value, value, 0); \
> +        xu(width, name, fixed_value, value, value, 0, ); \
>      } while (0)
>  
>  
> diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
> index 89512a26bb..4ff04ae52d 100644
> --- a/libavcodec/cbs_jpeg.c
> +++ b/libavcodec/cbs_jpeg.c
> @@ -34,7 +34,7 @@
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define u(width, name, range_min, range_max) \
> -    xu(width, name, range_min, range_max, 0)
> +    xu(width, name, range_min, range_max, 0, )
>  #define us(width, name, sub, range_min, range_max) \
>      xu(width, name, range_min, range_max, 1, sub)
>  
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index 0e5d08ecbf..97f7889cbb 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -41,9 +41,9 @@
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define ui(width, name) \
> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>  #define uir(width, name) \
> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
> +        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0, )
>  #define uis(width, name, subs, ...) \
>          xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>  #define uirs(width, name, subs, ...) \
> @@ -57,7 +57,7 @@
>          bit("marker_bit", 1)
>  #define bit(string, value) do { \
>          av_unused uint32_t bit = value; \
> -        xuia(1, string, bit, value, value, 0); \
> +        xuia(1, string, bit, value, value, 0, ); \
>      } while (0)
>  
>  
> diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
> index ec82f11c76..eef603bfb2 100644
> --- a/libavcodec/cbs_vp9.c
> +++ b/libavcodec/cbs_vp9.c
> @@ -253,15 +253,14 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define f(width, name) \
> -        xf(width, name, current->name, 0)
> +        xf(width, name, current->name, 0, )
>  #define s(width, name) \
> -        xs(width, name, current->name, 0)
> +        xs(width, name, current->name, 0, )
>  #define fs(width, name, subs, ...) \
>          xf(width, name, current->name, subs, __VA_ARGS__)
>  #define ss(width, name, subs, ...) \
>          xs(width, name, current->name, subs, __VA_ARGS__)
>  
> -
>  #define READ
>  #define READWRITE read
>  #define RWContext GetBitContext
> @@ -295,9 +294,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>  #define delta_q(name) do { \
>          uint8_t delta_coded; \
>          int8_t delta_q; \
> -        xf(1, name.delta_coded, delta_coded, 0); \
> +        xf(1, name.delta_coded, delta_coded, 0, ); \
>          if (delta_coded) \
> -            xs(4, name.delta_q, delta_q, 0); \
> +            xs(4, name.delta_q, delta_q, 0, ); \
>          else \
>              delta_q = 0; \
>          current->name = delta_q; \
> @@ -366,9 +365,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>      } while (0)
>  
>  #define delta_q(name) do { \
> -        xf(1, name.delta_coded, !!current->name, 0); \
> +        xf(1, name.delta_coded, !!current->name, 0, ); \
>          if (current->name) \
> -            xs(4, name.delta_q, current->name, 0); \
> +            xs(4, name.delta_q, current->name, 0, ); \
>      } while (0)
>  
>  #define prob(name, subs, ...) do { \
> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
> index 125eb02589..2f08eccf18 100644
> --- a/libavcodec/cbs_vp9_syntax_template.c
> +++ b/libavcodec/cbs_vp9_syntax_template.c
> @@ -19,23 +19,11 @@
>  static int FUNC(frame_sync_code)(CodedBitstreamContext *ctx, RWContext *rw,
>                                   VP9RawFrameHeader *current)
>  {
> -    uint8_t frame_sync_byte_0 = VP9_FRAME_SYNC_0;
> -    uint8_t frame_sync_byte_1 = VP9_FRAME_SYNC_1;
> -    uint8_t frame_sync_byte_2 = VP9_FRAME_SYNC_2;
>      int err;
>  
> -    xf(8, frame_sync_byte_0, frame_sync_byte_0, 0);
> -    xf(8, frame_sync_byte_1, frame_sync_byte_1, 0);
> -    xf(8, frame_sync_byte_2, frame_sync_byte_2, 0);
> -
> -    if (frame_sync_byte_0 != VP9_FRAME_SYNC_0 ||
> -        frame_sync_byte_1 != VP9_FRAME_SYNC_1 ||
> -        frame_sync_byte_2 != VP9_FRAME_SYNC_2) {
> -        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid frame sync code: "
> -               "%02x %02x %02x.\n", frame_sync_byte_0,
> -               frame_sync_byte_1, frame_sync_byte_2);
> -        return AVERROR_INVALIDDATA;
> -    }
> +    fixed(8, frame_sync_byte_0, VP9_FRAME_SYNC_0);
> +    fixed(8, frame_sync_byte_1, VP9_FRAME_SYNC_1);
> +    fixed(8, frame_sync_byte_2, VP9_FRAME_SYNC_2);
>  
>      return 0;
>  }
> @@ -396,9 +384,8 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>  static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext *rw)
>  {
>      int err;
> -    av_unused int zero = 0;
>      while (byte_alignment(rw) != 0)
> -        xf(1, zero_bit, zero, 0);
> +        fixed(1, zero_bit, 0);
>  
>      return 0;
>  }
> 
Ping.

- Andreas
Andreas Rheinhardt April 7, 2020, 12:51 p.m. UTC | #2
Andreas Rheinhardt:
> Andreas Rheinhardt:
>> According to C99, there has to be at least one argument for every ...
>> in a variadic function-like macro. In practice most (all?) compilers also
>> allow to leave it completely out, but it is nevertheless required: In a
>> variadic macro "there shall be more arguments in the invocation than there
>> are parameters in the macro definition (excluding the ...)." (C99,
>> 6.10.3.4).
>>
>> CBS (not the framework itself, but the macros used in the
>> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
>> a variadic macro argument out. This leads to warnings when compiling in
>> -pedantic mode, e.g. "warning: must specify at least one argument for
>> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
>> from Clang.
>>
>> Most of these warnings can be easily avoided: The syntax_templates
>> mostly contain helper macros that expand to more complex variadic macros
>> and these helper macros often omit an argument for the .... Modifying
>> them to always expand to complex macros with an empty argument for the
>> ... at the end fixes most of these warnings: The number of warnings went
>> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
>> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
>>
>> These eight remaining warnings for cbs_vp9 have been fixed by switching
>> to another macro in cbs_vp9_syntax_template: The fixed values for the
>> sync bytes as well as the trailing bits for byte-alignment are now read
>> via the fixed() macro (this also adds a check to ensure that trailing
>> bits are indeed zero as they have to be).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> There are two ways to fix the remaining 32 warnings from cbs_h2645:
>>
>> Simply add ", " to all macro calls that make use of the complex macros;
>> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
>>
>> Or add new macros for these macro calls: The places that produce
>> warnings use the complex macros directly, because they use names
>> different from the default names that the helper macros use, but they do
>> not use subscripts and therefore leave the variadic argument (designed
>> for subscripts) out. I would have implemented the second solution if it
>> were not for the problem of the naming of the new macros.
>>
>> (There is of course also the possibility not to care about the remaining
>> ones.)
>>
>>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>>  libavcodec/cbs_jpeg.c                |  2 +-
>>  libavcodec/cbs_mpeg2.c               |  6 +++---
>>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>>  6 files changed, 29 insertions(+), 43 deletions(-)
>>
>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>> index 16eb7143b6..fc228086c2 100644
>> --- a/libavcodec/cbs_av1.c
>> +++ b/libavcodec/cbs_av1.c
>> @@ -550,12 +550,12 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define fb(width, name) \
>> -        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>> +        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>  #define fc(width, name, range_min, range_max) \
>> -        xf(width, name, current->name, range_min, range_max, 0)
>> +        xf(width, name, current->name, range_min, range_max, 0, )
>>  #define flag(name) fb(1, name)
>>  #define su(width, name) \
>> -        xsu(width, name, current->name, 0)
>> +        xsu(width, name, current->name, 0, )
>>  
>>  #define fbs(width, name, subs, ...) \
>>          xf(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>> @@ -568,7 +568,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>  
>>  #define fixed(width, name, value) do { \
>>          av_unused uint32_t fixed_value = value; \
>> -        xf(width, name, fixed_value, value, value, 0); \
>> +        xf(width, name, fixed_value, value, value, 0, ); \
>>      } while (0)
>>  
>>  
>> @@ -623,9 +623,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>  #define delta_q(name) do { \
>>          uint8_t delta_coded; \
>>          int8_t delta_q; \
>> -        xf(1, name.delta_coded, delta_coded, 0, 1, 0); \
>> +        xf(1, name.delta_coded, delta_coded, 0, 1, 0, ); \
>>          if (delta_coded) \
>> -            xsu(1 + 6, name.delta_q, delta_q, 0); \
>> +            xsu(1 + 6, name.delta_q, delta_q, 0, ); \
>>          else \
>>              delta_q = 0; \
>>          current->name = delta_q; \
>> @@ -700,9 +700,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>      } while (0)
>>  
>>  #define delta_q(name) do { \
>> -        xf(1, name.delta_coded, current->name != 0, 0, 1, 0); \
>> +        xf(1, name.delta_coded, current->name != 0, 0, 1, 0, ); \
>>          if (current->name) \
>> -            xsu(1 + 6, name.delta_q, current->name, 0); \
>> +            xsu(1 + 6, name.delta_q, current->name, 0, ); \
>>      } while (0)
>>  
>>  #define leb128(name) do { \
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 7a4eecf439..d42073cc5a 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -250,18 +250,18 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define u(width, name, range_min, range_max) \
>> -        xu(width, name, current->name, range_min, range_max, 0)
>> +        xu(width, name, current->name, range_min, range_max, 0, )
>>  #define ub(width, name) \
>> -        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>> +        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>  #define flag(name) ub(1, name)
>>  #define ue(name, range_min, range_max) \
>> -        xue(name, current->name, range_min, range_max, 0)
>> +        xue(name, current->name, range_min, range_max, 0, )
>>  #define i(width, name, range_min, range_max) \
>> -        xi(width, name, current->name, range_min, range_max, 0)
>> +        xi(width, name, current->name, range_min, range_max, 0, )
>>  #define ib(width, name) \
>> -        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0)
>> +        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0, )
>>  #define se(name, range_min, range_max) \
>> -        xse(name, current->name, range_min, range_max, 0)
>> +        xse(name, current->name, range_min, range_max, 0, )
>>  
>>  #define us(width, name, range_min, range_max, subs, ...) \
>>          xu(width, name, current->name, range_min, range_max, subs, __VA_ARGS__)
>> @@ -280,7 +280,7 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>  
>>  #define fixed(width, name, value) do { \
>>          av_unused uint32_t fixed_value = value; \
>> -        xu(width, name, fixed_value, value, value, 0); \
>> +        xu(width, name, fixed_value, value, value, 0, ); \
>>      } while (0)
>>  
>>  
>> diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
>> index 89512a26bb..4ff04ae52d 100644
>> --- a/libavcodec/cbs_jpeg.c
>> +++ b/libavcodec/cbs_jpeg.c
>> @@ -34,7 +34,7 @@
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define u(width, name, range_min, range_max) \
>> -    xu(width, name, range_min, range_max, 0)
>> +    xu(width, name, range_min, range_max, 0, )
>>  #define us(width, name, sub, range_min, range_max) \
>>      xu(width, name, range_min, range_max, 1, sub)
>>  
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index 0e5d08ecbf..97f7889cbb 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -41,9 +41,9 @@
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define ui(width, name) \
>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>  #define uir(width, name) \
>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
>> +        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0, )
>>  #define uis(width, name, subs, ...) \
>>          xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>  #define uirs(width, name, subs, ...) \
>> @@ -57,7 +57,7 @@
>>          bit("marker_bit", 1)
>>  #define bit(string, value) do { \
>>          av_unused uint32_t bit = value; \
>> -        xuia(1, string, bit, value, value, 0); \
>> +        xuia(1, string, bit, value, value, 0, ); \
>>      } while (0)
>>  
>>  
>> diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
>> index ec82f11c76..eef603bfb2 100644
>> --- a/libavcodec/cbs_vp9.c
>> +++ b/libavcodec/cbs_vp9.c
>> @@ -253,15 +253,14 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define f(width, name) \
>> -        xf(width, name, current->name, 0)
>> +        xf(width, name, current->name, 0, )
>>  #define s(width, name) \
>> -        xs(width, name, current->name, 0)
>> +        xs(width, name, current->name, 0, )
>>  #define fs(width, name, subs, ...) \
>>          xf(width, name, current->name, subs, __VA_ARGS__)
>>  #define ss(width, name, subs, ...) \
>>          xs(width, name, current->name, subs, __VA_ARGS__)
>>  
>> -
>>  #define READ
>>  #define READWRITE read
>>  #define RWContext GetBitContext
>> @@ -295,9 +294,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>  #define delta_q(name) do { \
>>          uint8_t delta_coded; \
>>          int8_t delta_q; \
>> -        xf(1, name.delta_coded, delta_coded, 0); \
>> +        xf(1, name.delta_coded, delta_coded, 0, ); \
>>          if (delta_coded) \
>> -            xs(4, name.delta_q, delta_q, 0); \
>> +            xs(4, name.delta_q, delta_q, 0, ); \
>>          else \
>>              delta_q = 0; \
>>          current->name = delta_q; \
>> @@ -366,9 +365,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>      } while (0)
>>  
>>  #define delta_q(name) do { \
>> -        xf(1, name.delta_coded, !!current->name, 0); \
>> +        xf(1, name.delta_coded, !!current->name, 0, ); \
>>          if (current->name) \
>> -            xs(4, name.delta_q, current->name, 0); \
>> +            xs(4, name.delta_q, current->name, 0, ); \
>>      } while (0)
>>  
>>  #define prob(name, subs, ...) do { \
>> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
>> index 125eb02589..2f08eccf18 100644
>> --- a/libavcodec/cbs_vp9_syntax_template.c
>> +++ b/libavcodec/cbs_vp9_syntax_template.c
>> @@ -19,23 +19,11 @@
>>  static int FUNC(frame_sync_code)(CodedBitstreamContext *ctx, RWContext *rw,
>>                                   VP9RawFrameHeader *current)
>>  {
>> -    uint8_t frame_sync_byte_0 = VP9_FRAME_SYNC_0;
>> -    uint8_t frame_sync_byte_1 = VP9_FRAME_SYNC_1;
>> -    uint8_t frame_sync_byte_2 = VP9_FRAME_SYNC_2;
>>      int err;
>>  
>> -    xf(8, frame_sync_byte_0, frame_sync_byte_0, 0);
>> -    xf(8, frame_sync_byte_1, frame_sync_byte_1, 0);
>> -    xf(8, frame_sync_byte_2, frame_sync_byte_2, 0);
>> -
>> -    if (frame_sync_byte_0 != VP9_FRAME_SYNC_0 ||
>> -        frame_sync_byte_1 != VP9_FRAME_SYNC_1 ||
>> -        frame_sync_byte_2 != VP9_FRAME_SYNC_2) {
>> -        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid frame sync code: "
>> -               "%02x %02x %02x.\n", frame_sync_byte_0,
>> -               frame_sync_byte_1, frame_sync_byte_2);
>> -        return AVERROR_INVALIDDATA;
>> -    }
>> +    fixed(8, frame_sync_byte_0, VP9_FRAME_SYNC_0);
>> +    fixed(8, frame_sync_byte_1, VP9_FRAME_SYNC_1);
>> +    fixed(8, frame_sync_byte_2, VP9_FRAME_SYNC_2);
>>  
>>      return 0;
>>  }
>> @@ -396,9 +384,8 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>  static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext *rw)
>>  {
>>      int err;
>> -    av_unused int zero = 0;
>>      while (byte_alignment(rw) != 0)
>> -        xf(1, zero_bit, zero, 0);
>> +        fixed(1, zero_bit, 0);
>>  
>>      return 0;
>>  }
>>
> Ping.
> 
> - Andreas
> 
Ping.

- Andreas
Andreas Rheinhardt April 12, 2020, 12:42 p.m. UTC | #3
Andreas Rheinhardt:
> Andreas Rheinhardt:
>> Andreas Rheinhardt:
>>> According to C99, there has to be at least one argument for every ...
>>> in a variadic function-like macro. In practice most (all?) compilers also
>>> allow to leave it completely out, but it is nevertheless required: In a
>>> variadic macro "there shall be more arguments in the invocation than there
>>> are parameters in the macro definition (excluding the ...)." (C99,
>>> 6.10.3.4).
>>>
>>> CBS (not the framework itself, but the macros used in the
>>> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
>>> a variadic macro argument out. This leads to warnings when compiling in
>>> -pedantic mode, e.g. "warning: must specify at least one argument for
>>> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
>>> from Clang.
>>>
>>> Most of these warnings can be easily avoided: The syntax_templates
>>> mostly contain helper macros that expand to more complex variadic macros
>>> and these helper macros often omit an argument for the .... Modifying
>>> them to always expand to complex macros with an empty argument for the
>>> ... at the end fixes most of these warnings: The number of warnings went
>>> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
>>> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
>>>
>>> These eight remaining warnings for cbs_vp9 have been fixed by switching
>>> to another macro in cbs_vp9_syntax_template: The fixed values for the
>>> sync bytes as well as the trailing bits for byte-alignment are now read
>>> via the fixed() macro (this also adds a check to ensure that trailing
>>> bits are indeed zero as they have to be).
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>> There are two ways to fix the remaining 32 warnings from cbs_h2645:
>>>
>>> Simply add ", " to all macro calls that make use of the complex macros;
>>> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
>>>
>>> Or add new macros for these macro calls: The places that produce
>>> warnings use the complex macros directly, because they use names
>>> different from the default names that the helper macros use, but they do
>>> not use subscripts and therefore leave the variadic argument (designed
>>> for subscripts) out. I would have implemented the second solution if it
>>> were not for the problem of the naming of the new macros.
>>>
>>> (There is of course also the possibility not to care about the remaining
>>> ones.)
>>>
>>>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>>>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>>>  libavcodec/cbs_jpeg.c                |  2 +-
>>>  libavcodec/cbs_mpeg2.c               |  6 +++---
>>>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>>>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>>>  6 files changed, 29 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
>>> index 16eb7143b6..fc228086c2 100644
>>> --- a/libavcodec/cbs_av1.c
>>> +++ b/libavcodec/cbs_av1.c
>>> @@ -550,12 +550,12 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define fb(width, name) \
>>> -        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>>> +        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>>  #define fc(width, name, range_min, range_max) \
>>> -        xf(width, name, current->name, range_min, range_max, 0)
>>> +        xf(width, name, current->name, range_min, range_max, 0, )
>>>  #define flag(name) fb(1, name)
>>>  #define su(width, name) \
>>> -        xsu(width, name, current->name, 0)
>>> +        xsu(width, name, current->name, 0, )
>>>  
>>>  #define fbs(width, name, subs, ...) \
>>>          xf(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>> @@ -568,7 +568,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>>  
>>>  #define fixed(width, name, value) do { \
>>>          av_unused uint32_t fixed_value = value; \
>>> -        xf(width, name, fixed_value, value, value, 0); \
>>> +        xf(width, name, fixed_value, value, value, 0, ); \
>>>      } while (0)
>>>  
>>>  
>>> @@ -623,9 +623,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>>  #define delta_q(name) do { \
>>>          uint8_t delta_coded; \
>>>          int8_t delta_q; \
>>> -        xf(1, name.delta_coded, delta_coded, 0, 1, 0); \
>>> +        xf(1, name.delta_coded, delta_coded, 0, 1, 0, ); \
>>>          if (delta_coded) \
>>> -            xsu(1 + 6, name.delta_q, delta_q, 0); \
>>> +            xsu(1 + 6, name.delta_q, delta_q, 0, ); \
>>>          else \
>>>              delta_q = 0; \
>>>          current->name = delta_q; \
>>> @@ -700,9 +700,9 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
>>>      } while (0)
>>>  
>>>  #define delta_q(name) do { \
>>> -        xf(1, name.delta_coded, current->name != 0, 0, 1, 0); \
>>> +        xf(1, name.delta_coded, current->name != 0, 0, 1, 0, ); \
>>>          if (current->name) \
>>> -            xsu(1 + 6, name.delta_q, current->name, 0); \
>>> +            xsu(1 + 6, name.delta_q, current->name, 0, ); \
>>>      } while (0)
>>>  
>>>  #define leb128(name) do { \
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index 7a4eecf439..d42073cc5a 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -250,18 +250,18 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define u(width, name, range_min, range_max) \
>>> -        xu(width, name, current->name, range_min, range_max, 0)
>>> +        xu(width, name, current->name, range_min, range_max, 0, )
>>>  #define ub(width, name) \
>>> -        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>>> +        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>>  #define flag(name) ub(1, name)
>>>  #define ue(name, range_min, range_max) \
>>> -        xue(name, current->name, range_min, range_max, 0)
>>> +        xue(name, current->name, range_min, range_max, 0, )
>>>  #define i(width, name, range_min, range_max) \
>>> -        xi(width, name, current->name, range_min, range_max, 0)
>>> +        xi(width, name, current->name, range_min, range_max, 0, )
>>>  #define ib(width, name) \
>>> -        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0)
>>> +        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0, )
>>>  #define se(name, range_min, range_max) \
>>> -        xse(name, current->name, range_min, range_max, 0)
>>> +        xse(name, current->name, range_min, range_max, 0, )
>>>  
>>>  #define us(width, name, range_min, range_max, subs, ...) \
>>>          xu(width, name, current->name, range_min, range_max, subs, __VA_ARGS__)
>>> @@ -280,7 +280,7 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>  
>>>  #define fixed(width, name, value) do { \
>>>          av_unused uint32_t fixed_value = value; \
>>> -        xu(width, name, fixed_value, value, value, 0); \
>>> +        xu(width, name, fixed_value, value, value, 0, ); \
>>>      } while (0)
>>>  
>>>  
>>> diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
>>> index 89512a26bb..4ff04ae52d 100644
>>> --- a/libavcodec/cbs_jpeg.c
>>> +++ b/libavcodec/cbs_jpeg.c
>>> @@ -34,7 +34,7 @@
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define u(width, name, range_min, range_max) \
>>> -    xu(width, name, range_min, range_max, 0)
>>> +    xu(width, name, range_min, range_max, 0, )
>>>  #define us(width, name, sub, range_min, range_max) \
>>>      xu(width, name, range_min, range_max, 1, sub)
>>>  
>>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>>> index 0e5d08ecbf..97f7889cbb 100644
>>> --- a/libavcodec/cbs_mpeg2.c
>>> +++ b/libavcodec/cbs_mpeg2.c
>>> @@ -41,9 +41,9 @@
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define ui(width, name) \
>>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>>> +        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
>>>  #define uir(width, name) \
>>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
>>> +        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0, )
>>>  #define uis(width, name, subs, ...) \
>>>          xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>>  #define uirs(width, name, subs, ...) \
>>> @@ -57,7 +57,7 @@
>>>          bit("marker_bit", 1)
>>>  #define bit(string, value) do { \
>>>          av_unused uint32_t bit = value; \
>>> -        xuia(1, string, bit, value, value, 0); \
>>> +        xuia(1, string, bit, value, value, 0, ); \
>>>      } while (0)
>>>  
>>>  
>>> diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
>>> index ec82f11c76..eef603bfb2 100644
>>> --- a/libavcodec/cbs_vp9.c
>>> +++ b/libavcodec/cbs_vp9.c
>>> @@ -253,15 +253,14 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define f(width, name) \
>>> -        xf(width, name, current->name, 0)
>>> +        xf(width, name, current->name, 0, )
>>>  #define s(width, name) \
>>> -        xs(width, name, current->name, 0)
>>> +        xs(width, name, current->name, 0, )
>>>  #define fs(width, name, subs, ...) \
>>>          xf(width, name, current->name, subs, __VA_ARGS__)
>>>  #define ss(width, name, subs, ...) \
>>>          xs(width, name, current->name, subs, __VA_ARGS__)
>>>  
>>> -
>>>  #define READ
>>>  #define READWRITE read
>>>  #define RWContext GetBitContext
>>> @@ -295,9 +294,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>  #define delta_q(name) do { \
>>>          uint8_t delta_coded; \
>>>          int8_t delta_q; \
>>> -        xf(1, name.delta_coded, delta_coded, 0); \
>>> +        xf(1, name.delta_coded, delta_coded, 0, ); \
>>>          if (delta_coded) \
>>> -            xs(4, name.delta_q, delta_q, 0); \
>>> +            xs(4, name.delta_q, delta_q, 0, ); \
>>>          else \
>>>              delta_q = 0; \
>>>          current->name = delta_q; \
>>> @@ -366,9 +365,9 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
>>>      } while (0)
>>>  
>>>  #define delta_q(name) do { \
>>> -        xf(1, name.delta_coded, !!current->name, 0); \
>>> +        xf(1, name.delta_coded, !!current->name, 0, ); \
>>>          if (current->name) \
>>> -            xs(4, name.delta_q, current->name, 0); \
>>> +            xs(4, name.delta_q, current->name, 0, ); \
>>>      } while (0)
>>>  
>>>  #define prob(name, subs, ...) do { \
>>> diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
>>> index 125eb02589..2f08eccf18 100644
>>> --- a/libavcodec/cbs_vp9_syntax_template.c
>>> +++ b/libavcodec/cbs_vp9_syntax_template.c
>>> @@ -19,23 +19,11 @@
>>>  static int FUNC(frame_sync_code)(CodedBitstreamContext *ctx, RWContext *rw,
>>>                                   VP9RawFrameHeader *current)
>>>  {
>>> -    uint8_t frame_sync_byte_0 = VP9_FRAME_SYNC_0;
>>> -    uint8_t frame_sync_byte_1 = VP9_FRAME_SYNC_1;
>>> -    uint8_t frame_sync_byte_2 = VP9_FRAME_SYNC_2;
>>>      int err;
>>>  
>>> -    xf(8, frame_sync_byte_0, frame_sync_byte_0, 0);
>>> -    xf(8, frame_sync_byte_1, frame_sync_byte_1, 0);
>>> -    xf(8, frame_sync_byte_2, frame_sync_byte_2, 0);
>>> -
>>> -    if (frame_sync_byte_0 != VP9_FRAME_SYNC_0 ||
>>> -        frame_sync_byte_1 != VP9_FRAME_SYNC_1 ||
>>> -        frame_sync_byte_2 != VP9_FRAME_SYNC_2) {
>>> -        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid frame sync code: "
>>> -               "%02x %02x %02x.\n", frame_sync_byte_0,
>>> -               frame_sync_byte_1, frame_sync_byte_2);
>>> -        return AVERROR_INVALIDDATA;
>>> -    }
>>> +    fixed(8, frame_sync_byte_0, VP9_FRAME_SYNC_0);
>>> +    fixed(8, frame_sync_byte_1, VP9_FRAME_SYNC_1);
>>> +    fixed(8, frame_sync_byte_2, VP9_FRAME_SYNC_2);
>>>  
>>>      return 0;
>>>  }
>>> @@ -396,9 +384,8 @@ static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>>  static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext *rw)
>>>  {
>>>      int err;
>>> -    av_unused int zero = 0;
>>>      while (byte_alignment(rw) != 0)
>>> -        xf(1, zero_bit, zero, 0);
>>> +        fixed(1, zero_bit, 0);
>>>  
>>>      return 0;
>>>  }
>>>
>> Ping.
>>
>> - Andreas
>>
> Ping.
> 
> - Andreas
> 
Ping.

- Andreas
Mark Thompson April 12, 2020, 1:39 p.m. UTC | #4
On 23/03/2020 00:52, Andreas Rheinhardt wrote:
> According to C99, there has to be at least one argument for every ...
> in a variadic function-like macro. In practice most (all?) compilers also
> allow to leave it completely out, but it is nevertheless required: In a
> variadic macro "there shall be more arguments in the invocation than there
> are parameters in the macro definition (excluding the ...)." (C99,
> 6.10.3.4).
> 
> CBS (not the framework itself, but the macros used in the
> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
> a variadic macro argument out. This leads to warnings when compiling in
> -pedantic mode, e.g. "warning: must specify at least one argument for
> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
> from Clang.
> 
> Most of these warnings can be easily avoided: The syntax_templates
> mostly contain helper macros that expand to more complex variadic macros
> and these helper macros often omit an argument for the .... Modifying
> them to always expand to complex macros with an empty argument for the
> ... at the end fixes most of these warnings: The number of warnings went
> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
> 
> These eight remaining warnings for cbs_vp9 have been fixed by switching
> to another macro in cbs_vp9_syntax_template: The fixed values for the
> sync bytes as well as the trailing bits for byte-alignment are now read
> via the fixed() macro (this also adds a check to ensure that trailing
> bits are indeed zero as they have to be).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> There are two ways to fix the remaining 32 warnings from cbs_h2645:
> 
> Simply add ", " to all macro calls that make use of the complex macros;
> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
> 
> Or add new macros for these macro calls: The places that produce
> warnings use the complex macros directly, because they use names
> different from the default names that the helper macros use, but they do
> not use subscripts and therefore leave the variadic argument (designed
> for subscripts) out. I would have implemented the second solution if it
> were not for the problem of the naming of the new macros.
> 
> (There is of course also the possibility not to care about the remaining
> ones.)
> 
>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>  libavcodec/cbs_jpeg.c                |  2 +-
>  libavcodec/cbs_mpeg2.c               |  6 +++---
>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>  6 files changed, 29 insertions(+), 43 deletions(-)

Looks fine to me, keeping the ugliness in the macros rather than the templates is good.

Is there any compiler which actually fails here, or is the only case which finds it the warning in clang pedantic mode?

Thanks,

- Mark
Andreas Rheinhardt April 12, 2020, 1:51 p.m. UTC | #5
Mark Thompson:
> On 23/03/2020 00:52, Andreas Rheinhardt wrote:
>> According to C99, there has to be at least one argument for every ...
>> in a variadic function-like macro. In practice most (all?) compilers also
>> allow to leave it completely out, but it is nevertheless required: In a
>> variadic macro "there shall be more arguments in the invocation than there
>> are parameters in the macro definition (excluding the ...)." (C99,
>> 6.10.3.4).
>>
>> CBS (not the framework itself, but the macros used in the
>> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
>> a variadic macro argument out. This leads to warnings when compiling in
>> -pedantic mode, e.g. "warning: must specify at least one argument for
>> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
>> from Clang.
>>
>> Most of these warnings can be easily avoided: The syntax_templates
>> mostly contain helper macros that expand to more complex variadic macros
>> and these helper macros often omit an argument for the .... Modifying
>> them to always expand to complex macros with an empty argument for the
>> ... at the end fixes most of these warnings: The number of warnings went
>> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
>> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
>>
>> These eight remaining warnings for cbs_vp9 have been fixed by switching
>> to another macro in cbs_vp9_syntax_template: The fixed values for the
>> sync bytes as well as the trailing bits for byte-alignment are now read
>> via the fixed() macro (this also adds a check to ensure that trailing
>> bits are indeed zero as they have to be).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> There are two ways to fix the remaining 32 warnings from cbs_h2645:
>>
>> Simply add ", " to all macro calls that make use of the complex macros;
>> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
>>
>> Or add new macros for these macro calls: The places that produce
>> warnings use the complex macros directly, because they use names
>> different from the default names that the helper macros use, but they do
>> not use subscripts and therefore leave the variadic argument (designed
>> for subscripts) out. I would have implemented the second solution if it
>> were not for the problem of the naming of the new macros.
>>
>> (There is of course also the possibility not to care about the remaining
>> ones.)
>>
>>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>>  libavcodec/cbs_jpeg.c                |  2 +-
>>  libavcodec/cbs_mpeg2.c               |  6 +++---
>>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>>  6 files changed, 29 insertions(+), 43 deletions(-)
> 
> Looks fine to me, keeping the ugliness in the macros rather than the templates is good.
> 
> Is there any compiler which actually fails here, or is the only case which finds it the warning in clang pedantic mode?
> 
Both Clang as well as GCC warn in pedantic mode for this (although GCC's
warning is not called "gnu-zero-variadic-macro-arguments"; it simply
says "warning: ISO C99 requires at least one argument for the "..." in a
variadic macro").

Any suggestions for macro names for the remaining stuff (stuff that uses
an unusual name, but no subscripts (like last_payload_type_byte and
last_payload_size_byte))? If not, I'll apply.

I know of no compiler that refuses to compile because of this.

- Andreas
Andreas Rheinhardt April 12, 2020, 9:30 p.m. UTC | #6
Mark Thompson:
> On 23/03/2020 00:52, Andreas Rheinhardt wrote:
>> According to C99, there has to be at least one argument for every ...
>> in a variadic function-like macro. In practice most (all?) compilers also
>> allow to leave it completely out, but it is nevertheless required: In a
>> variadic macro "there shall be more arguments in the invocation than there
>> are parameters in the macro definition (excluding the ...)." (C99,
>> 6.10.3.4).
>>
>> CBS (not the framework itself, but the macros used in the
>> cbs_*_syntax_template.c files) relies on the compiler allowing to leave
>> a variadic macro argument out. This leads to warnings when compiling in
>> -pedantic mode, e.g. "warning: must specify at least one argument for
>> '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]"
>> from Clang.
>>
>> Most of these warnings can be easily avoided: The syntax_templates
>> mostly contain helper macros that expand to more complex variadic macros
>> and these helper macros often omit an argument for the .... Modifying
>> them to always expand to complex macros with an empty argument for the
>> ... at the end fixes most of these warnings: The number of warnings went
>> down from 400 to 0 for cbs_av1, from 1114 to 32 for cbs_h2645, from 38 to
>> 0 for cbs_jpeg, from 166 to 0 for cbs_mpeg2 and from 110 to 8 for cbs_vp9.
>>
>> These eight remaining warnings for cbs_vp9 have been fixed by switching
>> to another macro in cbs_vp9_syntax_template: The fixed values for the
>> sync bytes as well as the trailing bits for byte-alignment are now read
>> via the fixed() macro (this also adds a check to ensure that trailing
>> bits are indeed zero as they have to be).
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>> There are two ways to fix the remaining 32 warnings from cbs_h2645:
>>
>> Simply add ", " to all macro calls that make use of the complex macros;
>> this has the drawback of adding uglyness to cbs_h26x_syntax_template.c.
>>
>> Or add new macros for these macro calls: The places that produce
>> warnings use the complex macros directly, because they use names
>> different from the default names that the helper macros use, but they do
>> not use subscripts and therefore leave the variadic argument (designed
>> for subscripts) out. I would have implemented the second solution if it
>> were not for the problem of the naming of the new macros.
>>
>> (There is of course also the possibility not to care about the remaining
>> ones.)
>>
>>  libavcodec/cbs_av1.c                 | 16 ++++++++--------
>>  libavcodec/cbs_h2645.c               | 14 +++++++-------
>>  libavcodec/cbs_jpeg.c                |  2 +-
>>  libavcodec/cbs_mpeg2.c               |  6 +++---
>>  libavcodec/cbs_vp9.c                 | 13 ++++++-------
>>  libavcodec/cbs_vp9_syntax_template.c | 21 ++++-----------------
>>  6 files changed, 29 insertions(+), 43 deletions(-)
> 
> Looks fine to me, keeping the ugliness in the macros rather than the templates is good.
> 
> Is there any compiler which actually fails here, or is the only case which finds it the warning in clang pedantic mode?
> 
> Thanks,
> 
> - Mark

Applied, thanks.

- Andreas
diff mbox series

Patch

diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
index 16eb7143b6..fc228086c2 100644
--- a/libavcodec/cbs_av1.c
+++ b/libavcodec/cbs_av1.c
@@ -550,12 +550,12 @@  static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define fb(width, name) \
-        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
+        xf(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
 #define fc(width, name, range_min, range_max) \
-        xf(width, name, current->name, range_min, range_max, 0)
+        xf(width, name, current->name, range_min, range_max, 0, )
 #define flag(name) fb(1, name)
 #define su(width, name) \
-        xsu(width, name, current->name, 0)
+        xsu(width, name, current->name, 0, )
 
 #define fbs(width, name, subs, ...) \
         xf(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
@@ -568,7 +568,7 @@  static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
 
 #define fixed(width, name, value) do { \
         av_unused uint32_t fixed_value = value; \
-        xf(width, name, fixed_value, value, value, 0); \
+        xf(width, name, fixed_value, value, value, 0, ); \
     } while (0)
 
 
@@ -623,9 +623,9 @@  static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
 #define delta_q(name) do { \
         uint8_t delta_coded; \
         int8_t delta_q; \
-        xf(1, name.delta_coded, delta_coded, 0, 1, 0); \
+        xf(1, name.delta_coded, delta_coded, 0, 1, 0, ); \
         if (delta_coded) \
-            xsu(1 + 6, name.delta_q, delta_q, 0); \
+            xsu(1 + 6, name.delta_q, delta_q, 0, ); \
         else \
             delta_q = 0; \
         current->name = delta_q; \
@@ -700,9 +700,9 @@  static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc)
     } while (0)
 
 #define delta_q(name) do { \
-        xf(1, name.delta_coded, current->name != 0, 0, 1, 0); \
+        xf(1, name.delta_coded, current->name != 0, 0, 1, 0, ); \
         if (current->name) \
-            xsu(1 + 6, name.delta_q, current->name, 0); \
+            xsu(1 + 6, name.delta_q, current->name, 0, ); \
     } while (0)
 
 #define leb128(name) do { \
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
index 7a4eecf439..d42073cc5a 100644
--- a/libavcodec/cbs_h2645.c
+++ b/libavcodec/cbs_h2645.c
@@ -250,18 +250,18 @@  static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define u(width, name, range_min, range_max) \
-        xu(width, name, current->name, range_min, range_max, 0)
+        xu(width, name, current->name, range_min, range_max, 0, )
 #define ub(width, name) \
-        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
+        xu(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
 #define flag(name) ub(1, name)
 #define ue(name, range_min, range_max) \
-        xue(name, current->name, range_min, range_max, 0)
+        xue(name, current->name, range_min, range_max, 0, )
 #define i(width, name, range_min, range_max) \
-        xi(width, name, current->name, range_min, range_max, 0)
+        xi(width, name, current->name, range_min, range_max, 0, )
 #define ib(width, name) \
-        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0)
+        xi(width, name, current->name, MIN_INT_BITS(width), MAX_INT_BITS(width), 0, )
 #define se(name, range_min, range_max) \
-        xse(name, current->name, range_min, range_max, 0)
+        xse(name, current->name, range_min, range_max, 0, )
 
 #define us(width, name, range_min, range_max, subs, ...) \
         xu(width, name, current->name, range_min, range_max, subs, __VA_ARGS__)
@@ -280,7 +280,7 @@  static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc,
 
 #define fixed(width, name, value) do { \
         av_unused uint32_t fixed_value = value; \
-        xu(width, name, fixed_value, value, value, 0); \
+        xu(width, name, fixed_value, value, value, 0, ); \
     } while (0)
 
 
diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c
index 89512a26bb..4ff04ae52d 100644
--- a/libavcodec/cbs_jpeg.c
+++ b/libavcodec/cbs_jpeg.c
@@ -34,7 +34,7 @@ 
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define u(width, name, range_min, range_max) \
-    xu(width, name, range_min, range_max, 0)
+    xu(width, name, range_min, range_max, 0, )
 #define us(width, name, sub, range_min, range_max) \
     xu(width, name, range_min, range_max, 1, sub)
 
diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
index 0e5d08ecbf..97f7889cbb 100644
--- a/libavcodec/cbs_mpeg2.c
+++ b/libavcodec/cbs_mpeg2.c
@@ -41,9 +41,9 @@ 
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define ui(width, name) \
-        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
+        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0, )
 #define uir(width, name) \
-        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
+        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0, )
 #define uis(width, name, subs, ...) \
         xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
 #define uirs(width, name, subs, ...) \
@@ -57,7 +57,7 @@ 
         bit("marker_bit", 1)
 #define bit(string, value) do { \
         av_unused uint32_t bit = value; \
-        xuia(1, string, bit, value, value, 0); \
+        xuia(1, string, bit, value, value, 0, ); \
     } while (0)
 
 
diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c
index ec82f11c76..eef603bfb2 100644
--- a/libavcodec/cbs_vp9.c
+++ b/libavcodec/cbs_vp9.c
@@ -253,15 +253,14 @@  static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define f(width, name) \
-        xf(width, name, current->name, 0)
+        xf(width, name, current->name, 0, )
 #define s(width, name) \
-        xs(width, name, current->name, 0)
+        xs(width, name, current->name, 0, )
 #define fs(width, name, subs, ...) \
         xf(width, name, current->name, subs, __VA_ARGS__)
 #define ss(width, name, subs, ...) \
         xs(width, name, current->name, subs, __VA_ARGS__)
 
-
 #define READ
 #define READWRITE read
 #define RWContext GetBitContext
@@ -295,9 +294,9 @@  static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
 #define delta_q(name) do { \
         uint8_t delta_coded; \
         int8_t delta_q; \
-        xf(1, name.delta_coded, delta_coded, 0); \
+        xf(1, name.delta_coded, delta_coded, 0, ); \
         if (delta_coded) \
-            xs(4, name.delta_q, delta_q, 0); \
+            xs(4, name.delta_q, delta_q, 0, ); \
         else \
             delta_q = 0; \
         current->name = delta_q; \
@@ -366,9 +365,9 @@  static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc,
     } while (0)
 
 #define delta_q(name) do { \
-        xf(1, name.delta_coded, !!current->name, 0); \
+        xf(1, name.delta_coded, !!current->name, 0, ); \
         if (current->name) \
-            xs(4, name.delta_q, current->name, 0); \
+            xs(4, name.delta_q, current->name, 0, ); \
     } while (0)
 
 #define prob(name, subs, ...) do { \
diff --git a/libavcodec/cbs_vp9_syntax_template.c b/libavcodec/cbs_vp9_syntax_template.c
index 125eb02589..2f08eccf18 100644
--- a/libavcodec/cbs_vp9_syntax_template.c
+++ b/libavcodec/cbs_vp9_syntax_template.c
@@ -19,23 +19,11 @@ 
 static int FUNC(frame_sync_code)(CodedBitstreamContext *ctx, RWContext *rw,
                                  VP9RawFrameHeader *current)
 {
-    uint8_t frame_sync_byte_0 = VP9_FRAME_SYNC_0;
-    uint8_t frame_sync_byte_1 = VP9_FRAME_SYNC_1;
-    uint8_t frame_sync_byte_2 = VP9_FRAME_SYNC_2;
     int err;
 
-    xf(8, frame_sync_byte_0, frame_sync_byte_0, 0);
-    xf(8, frame_sync_byte_1, frame_sync_byte_1, 0);
-    xf(8, frame_sync_byte_2, frame_sync_byte_2, 0);
-
-    if (frame_sync_byte_0 != VP9_FRAME_SYNC_0 ||
-        frame_sync_byte_1 != VP9_FRAME_SYNC_1 ||
-        frame_sync_byte_2 != VP9_FRAME_SYNC_2) {
-        av_log(ctx->log_ctx, AV_LOG_ERROR, "Invalid frame sync code: "
-               "%02x %02x %02x.\n", frame_sync_byte_0,
-               frame_sync_byte_1, frame_sync_byte_2);
-        return AVERROR_INVALIDDATA;
-    }
+    fixed(8, frame_sync_byte_0, VP9_FRAME_SYNC_0);
+    fixed(8, frame_sync_byte_1, VP9_FRAME_SYNC_1);
+    fixed(8, frame_sync_byte_2, VP9_FRAME_SYNC_2);
 
     return 0;
 }
@@ -396,9 +384,8 @@  static int FUNC(uncompressed_header)(CodedBitstreamContext *ctx, RWContext *rw,
 static int FUNC(trailing_bits)(CodedBitstreamContext *ctx, RWContext *rw)
 {
     int err;
-    av_unused int zero = 0;
     while (byte_alignment(rw) != 0)
-        xf(1, zero_bit, zero, 0);
+        fixed(1, zero_bit, 0);
 
     return 0;
 }