diff mbox series

[FFmpeg-devel,v4,06/10] libavformat/asfdec: fix macro definition and use

Message ID 33b3d163dfcd61ae6f4ac258ae28fa0756436587.1652561722.git.ffmpegagent@gmail.com
State New
Headers show
Series libavformat/asf: fix handling of byte array length values | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Aman Karmani May 14, 2022, 8:55 p.m. UTC 
From: softworkz <softworkz@hotmail.com>

Signed-off-by: softworkz <softworkz@hotmail.com>
---
 libavformat/asfdec_f.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

Comments

Andreas Rheinhardt May 15, 2022, 6:12 p.m. UTC | #1 
softworkz:
> From: softworkz <softworkz@hotmail.com>
> 
> Signed-off-by: softworkz <softworkz@hotmail.com>
> ---
>  libavformat/asfdec_f.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> index 81a29f99d5..91c3874ac7 100644
> --- a/libavformat/asfdec_f.c
> +++ b/libavformat/asfdec_f.c
> @@ -906,21 +906,21 @@ static int asf_read_header(AVFormatContext *s)
>  }
>  
>  #define DO_2BITS(bits, var, defval)             \
> -    switch (bits & 3) {                         \
> +    switch ((bits) & 3) {                       \
>      case 3:                                     \
> -        var = avio_rl32(pb);                    \
> +        (var) = avio_rl32(pb);                  \
>          rsize += 4;                             \
>          break;                                  \
>      case 2:                                     \
> -        var = avio_rl16(pb);                    \
> +        (var) = avio_rl16(pb);                  \
>          rsize += 2;                             \
>          break;                                  \
>      case 1:                                     \
> -        var = avio_r8(pb);                      \
> +        (var) = avio_r8(pb);                    \
>          rsize++;                                \
>          break;                                  \
>      default:                                    \
> -        var = defval;                           \
> +        (var) = (defval);                       \
>          break;                                  \
>      }
>  
> @@ -1003,9 +1003,9 @@ static int asf_get_packet(AVFormatContext *s, AVIOContext *pb)
>      asf->packet_flags    = c;
>      asf->packet_property = d;
>  
> -    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size);
> -    DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence ignored
> -    DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding length
> +    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size)
> +    DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence ignored
> +    DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding length
>  
>      // the following checks prevent overflows and infinite loops
>      if (!packet_length || packet_length >= (1U << 29)) {
> @@ -1066,9 +1066,9 @@ static int asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
>      asf->stream_index     = asf->asfid2avid[num & 0x7f];
>      asfst                 = &asf->streams[num & 0x7f];
>      // sequence should be ignored!
> -    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
> -    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0);
> -    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
> +    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
> +    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0)
> +    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
>      av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d replic_size:%d num:%X packet_property %X\n",
>              asf->packet_key_frame, asf->stream_index, asf->packet_seq,
>              asf->packet_frag_offset, asf->packet_replic_size, num, asf->packet_property);
> @@ -1144,7 +1144,7 @@ static int asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
>          return AVERROR_INVALIDDATA;
>      }
>      if (asf->packet_flags & 0x01) {
> -        DO_2BITS(asf->packet_segsizetype >> 6, asf->packet_frag_size, 0); // 0 is illegal
> +        DO_2BITS(asf->packet_segsizetype >> 6, asf->packet_frag_size, 0) // 0 is illegal
>          if (rsize > asf->packet_size_left) {
>              av_log(s, AV_LOG_ERROR, "packet_replic_size is invalid\n");
>              return AVERROR_INVALIDDATA;

While protecting macro arguments is good, it is not really a "fix"
unless current usage is buggy. Which it isn't here, because >> has
higher precedence than &.
Furthermore I am not really sure whether removing the ';' is even
something worthwhile; they are surely unnecessary (being null
statements), but does this matter?

- Andreas
Soft Works May 15, 2022, 10:51 p.m. UTC | #2 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Sunday, May 15, 2022 8:12 PM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix
> macro definition and use
> 
> softworkz:
> > From: softworkz <softworkz@hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz@hotmail.com>
> > ---
> >  libavformat/asfdec_f.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> > index 81a29f99d5..91c3874ac7 100644
> > --- a/libavformat/asfdec_f.c
> > +++ b/libavformat/asfdec_f.c
> > @@ -906,21 +906,21 @@ static int asf_read_header(AVFormatContext *s)
> >  }
> >
> >  #define DO_2BITS(bits, var, defval)             \
> > -    switch (bits & 3) {                         \
> > +    switch ((bits) & 3) {                       \
> >      case 3:                                     \
> > -        var = avio_rl32(pb);                    \
> > +        (var) = avio_rl32(pb);                  \
> >          rsize += 4;                             \
> >          break;                                  \
> >      case 2:                                     \
> > -        var = avio_rl16(pb);                    \
> > +        (var) = avio_rl16(pb);                  \
> >          rsize += 2;                             \
> >          break;                                  \
> >      case 1:                                     \
> > -        var = avio_r8(pb);                      \
> > +        (var) = avio_r8(pb);                    \
> >          rsize++;                                \
> >          break;                                  \
> >      default:                                    \
> > -        var = defval;                           \
> > +        (var) = (defval);                       \
> >          break;                                  \
> >      }
> >
> > @@ -1003,9 +1003,9 @@ static int asf_get_packet(AVFormatContext *s,
> AVIOContext *pb)
> >      asf->packet_flags    = c;
> >      asf->packet_property = d;
> >
> > -    DO_2BITS(asf->packet_flags >> 5, packet_length, s-
> >packet_size);
> > -    DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence
> ignored
> > -    DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding length
> > +    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size)
> > +    DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence
> ignored
> > +    DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding length
> >
> >      // the following checks prevent overflows and infinite loops
> >      if (!packet_length || packet_length >= (1U << 29)) {
> > @@ -1066,9 +1066,9 @@ static int
> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >      asf->stream_index     = asf->asfid2avid[num & 0x7f];
> >      asfst                 = &asf->streams[num & 0x7f];
> >      // sequence should be ignored!
> > -    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
> > -    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
> 0);
> > -    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
> > +    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
> > +    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0)
> > +    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
> >      av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d
> replic_size:%d num:%X packet_property %X\n",
> >              asf->packet_key_frame, asf->stream_index, asf-
> >packet_seq,
> >              asf->packet_frag_offset, asf->packet_replic_size, num,
> asf->packet_property);
> > @@ -1144,7 +1144,7 @@ static int
> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >          return AVERROR_INVALIDDATA;
> >      }
> >      if (asf->packet_flags & 0x01) {
> > -        DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >packet_frag_size, 0); // 0 is illegal
> > +        DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >packet_frag_size, 0) // 0 is illegal
> >          if (rsize > asf->packet_size_left) {
> >              av_log(s, AV_LOG_ERROR, "packet_replic_size is
> invalid\n");
> >              return AVERROR_INVALIDDATA;
> 
> While protecting macro arguments is good, it is not really a "fix"
> unless current usage is buggy.

Ok, I will rephrase the commit message.

> Which it isn't here, because >> has higher precedence than &.

Could you explain which change you are referring to?

All this patch does is to put macro variables in brackets
and remove semicolons..

> Furthermore I am not really sure whether removing the ';' is even
> something worthwhile; they are surely unnecessary (being null
> statements), but does this matter?

It causes a warning

https://releases.llvm.org/13.0.0/tools/clang/docs/DiagnosticsReference.html#wextra-semi-stmt

I don't know how others are working, but I use to work in a way where
such warnings are shown in the editor and in lists in the IDE
even without compilation. Now - when you have a code file that
generates like 20, 50 or more warnings, it's much harder to spot
those warnings that might be really relevant and hinting at a mistake,
and you might be just too lazy to go through them each time.

The clang diagnostics have been helpful in spotting some actual 
issues in this very file. That's why I consider it worthwhile
to also eliminate such "non-issues".

Thanks for reviewing,
softworkz
Andreas Rheinhardt May 16, 2022, 8:48 a.m. UTC | #3 
Soft Works:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> Andreas Rheinhardt
>> Sent: Sunday, May 15, 2022 8:12 PM
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix
>> macro definition and use
>>
>> softworkz:
>>> From: softworkz <softworkz@hotmail.com>
>>>
>>> Signed-off-by: softworkz <softworkz@hotmail.com>
>>> ---
>>>  libavformat/asfdec_f.c | 24 ++++++++++++------------
>>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
>>> index 81a29f99d5..91c3874ac7 100644
>>> --- a/libavformat/asfdec_f.c
>>> +++ b/libavformat/asfdec_f.c
>>> @@ -906,21 +906,21 @@ static int asf_read_header(AVFormatContext *s)
>>>  }
>>>
>>>  #define DO_2BITS(bits, var, defval)             \
>>> -    switch (bits & 3) {                         \
>>> +    switch ((bits) & 3) {                       \
>>>      case 3:                                     \
>>> -        var = avio_rl32(pb);                    \
>>> +        (var) = avio_rl32(pb);                  \
>>>          rsize += 4;                             \
>>>          break;                                  \
>>>      case 2:                                     \
>>> -        var = avio_rl16(pb);                    \
>>> +        (var) = avio_rl16(pb);                  \
>>>          rsize += 2;                             \
>>>          break;                                  \
>>>      case 1:                                     \
>>> -        var = avio_r8(pb);                      \
>>> +        (var) = avio_r8(pb);                    \
>>>          rsize++;                                \
>>>          break;                                  \
>>>      default:                                    \
>>> -        var = defval;                           \
>>> +        (var) = (defval);                       \
>>>          break;                                  \
>>>      }
>>>
>>> @@ -1003,9 +1003,9 @@ static int asf_get_packet(AVFormatContext *s,
>> AVIOContext *pb)
>>>      asf->packet_flags    = c;
>>>      asf->packet_property = d;
>>>
>>> -    DO_2BITS(asf->packet_flags >> 5, packet_length, s-
>>> packet_size);
>>> -    DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence
>> ignored
>>> -    DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding length
>>> +    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size)
>>> +    DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence
>> ignored
>>> +    DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding length
>>>
>>>      // the following checks prevent overflows and infinite loops
>>>      if (!packet_length || packet_length >= (1U << 29)) {
>>> @@ -1066,9 +1066,9 @@ static int
>> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
>>>      asf->stream_index     = asf->asfid2avid[num & 0x7f];
>>>      asfst                 = &asf->streams[num & 0x7f];
>>>      // sequence should be ignored!
>>> -    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
>>> -    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
>> 0);
>>> -    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
>>> +    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
>>> +    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0)
>>> +    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
>>>      av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d
>> replic_size:%d num:%X packet_property %X\n",
>>>              asf->packet_key_frame, asf->stream_index, asf-
>>> packet_seq,
>>>              asf->packet_frag_offset, asf->packet_replic_size, num,
>> asf->packet_property);
>>> @@ -1144,7 +1144,7 @@ static int
>> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
>>>          return AVERROR_INVALIDDATA;
>>>      }
>>>      if (asf->packet_flags & 0x01) {
>>> -        DO_2BITS(asf->packet_segsizetype >> 6, asf-
>>> packet_frag_size, 0); // 0 is illegal
>>> +        DO_2BITS(asf->packet_segsizetype >> 6, asf-
>>> packet_frag_size, 0) // 0 is illegal
>>>          if (rsize > asf->packet_size_left) {
>>>              av_log(s, AV_LOG_ERROR, "packet_replic_size is
>> invalid\n");
>>>              return AVERROR_INVALIDDATA;
>>
>> While protecting macro arguments is good, it is not really a "fix"
>> unless current usage is buggy.
> 
> Ok, I will rephrase the commit message.
> 
>> Which it isn't here, because >> has higher precedence than &.
> 
> Could you explain which change you are referring to?
> 

Putting "bits" in parentheses. It doesn't change anything, because >>
has higher precedence than &.

> All this patch does is to put macro variables in brackets
> and remove semicolons..
> 
>> Furthermore I am not really sure whether removing the ';' is even
>> something worthwhile; they are surely unnecessary (being null
>> statements), but does this matter?
> 
> It causes a warning
> 
> https://releases.llvm.org/13.0.0/tools/clang/docs/DiagnosticsReference.html#wextra-semi-stmt
> 

I don't receive this warning despite using Clang 13.0. Do you have -Wall
or -Wextra or something like that enabled?
IMO a better fix for this would be to wrap the macro in a do {} while
(0) to keep the macro calls function-like.
Anyway, you should have mentioned in the commit message that your aim is
to fix this uncommon warning.

> I don't know how others are working, but I use to work in a way where
> such warnings are shown in the editor and in lists in the IDE
> even without compilation. Now - when you have a code file that
> generates like 20, 50 or more warnings, it's much harder to spot
> those warnings that might be really relevant and hinting at a mistake,
> and you might be just too lazy to go through them each time.
> 
> The clang diagnostics have been helpful in spotting some actual 
> issues in this very file. That's why I consider it worthwhile
> to also eliminate such "non-issues".
> 

I also work like that; e.g. my recent ac3.h header patchset was inspired
by clangd not liking cycles in header inclusions ("In included file:
main file cannot be included recursively when building a preamble").

- Andreas
Soft Works May 16, 2022, 10:03 p.m. UTC | #4 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Monday, May 16, 2022 10:49 AM
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix
> macro definition and use
> 
> Soft Works:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> >> Andreas Rheinhardt
> >> Sent: Sunday, May 15, 2022 8:12 PM
> >> To: ffmpeg-devel@ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec:
> fix
> >> macro definition and use
> >>
> >> softworkz:
> >>> From: softworkz <softworkz@hotmail.com>
> >>>
> >>> Signed-off-by: softworkz <softworkz@hotmail.com>
> >>> ---
> >>>  libavformat/asfdec_f.c | 24 ++++++++++++------------
> >>>  1 file changed, 12 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
> >>> index 81a29f99d5..91c3874ac7 100644
> >>> --- a/libavformat/asfdec_f.c
> >>> +++ b/libavformat/asfdec_f.c
> >>> @@ -906,21 +906,21 @@ static int asf_read_header(AVFormatContext
> *s)
> >>>  }
> >>>
> >>>  #define DO_2BITS(bits, var, defval)             \
> >>> -    switch (bits & 3) {                         \
> >>> +    switch ((bits) & 3) {                       \
> >>>      case 3:                                     \
> >>> -        var = avio_rl32(pb);                    \
> >>> +        (var) = avio_rl32(pb);                  \
> >>>          rsize += 4;                             \
> >>>          break;                                  \
> >>>      case 2:                                     \
> >>> -        var = avio_rl16(pb);                    \
> >>> +        (var) = avio_rl16(pb);                  \
> >>>          rsize += 2;                             \
> >>>          break;                                  \
> >>>      case 1:                                     \
> >>> -        var = avio_r8(pb);                      \
> >>> +        (var) = avio_r8(pb);                    \
> >>>          rsize++;                                \
> >>>          break;                                  \
> >>>      default:                                    \
> >>> -        var = defval;                           \
> >>> +        (var) = (defval);                       \
> >>>          break;                                  \
> >>>      }
> >>>
> >>> @@ -1003,9 +1003,9 @@ static int asf_get_packet(AVFormatContext
> *s,
> >> AVIOContext *pb)
> >>>      asf->packet_flags    = c;
> >>>      asf->packet_property = d;
> >>>
> >>> -    DO_2BITS(asf->packet_flags >> 5, packet_length, s-
> >>> packet_size);
> >>> -    DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence
> >> ignored
> >>> -    DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding
> length
> >>> +    DO_2BITS(asf->packet_flags >> 5, packet_length, s-
> >packet_size)
> >>> +    DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence
> >> ignored
> >>> +    DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding
> length
> >>>
> >>>      // the following checks prevent overflows and infinite loops
> >>>      if (!packet_length || packet_length >= (1U << 29)) {
> >>> @@ -1066,9 +1066,9 @@ static int
> >> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >>>      asf->stream_index     = asf->asfid2avid[num & 0x7f];
> >>>      asfst                 = &asf->streams[num & 0x7f];
> >>>      // sequence should be ignored!
> >>> -    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
> >>> -    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
> >> 0);
> >>> -    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
> >>> +    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
> >>> +    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset,
> 0)
> >>> +    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
> >>>      av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d
> >> replic_size:%d num:%X packet_property %X\n",
> >>>              asf->packet_key_frame, asf->stream_index, asf-
> >>> packet_seq,
> >>>              asf->packet_frag_offset, asf->packet_replic_size,
> num,
> >> asf->packet_property);
> >>> @@ -1144,7 +1144,7 @@ static int
> >> asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
> >>>          return AVERROR_INVALIDDATA;
> >>>      }
> >>>      if (asf->packet_flags & 0x01) {
> >>> -        DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >>> packet_frag_size, 0); // 0 is illegal
> >>> +        DO_2BITS(asf->packet_segsizetype >> 6, asf-
> >>> packet_frag_size, 0) // 0 is illegal
> >>>          if (rsize > asf->packet_size_left) {
> >>>              av_log(s, AV_LOG_ERROR, "packet_replic_size is
> >> invalid\n");
> >>>              return AVERROR_INVALIDDATA;
> >>
> >> While protecting macro arguments is good, it is not really a "fix"
> >> unless current usage is buggy.
> >
> > Ok, I will rephrase the commit message.
> >
> >> Which it isn't here, because >> has higher precedence than &.
> >
> > Could you explain which change you are referring to?
> >
> 
> Putting "bits" in parentheses. It doesn't change anything, because >>
> has higher precedence than &.

Ah, that's what you mean. I didn't even look at the usages of
the macro, because I think a macro should be safe intrinsically,
not only based on its current usages.

Actually this had also caught my attention due to a clang warning:
https://releases.llvm.org/13.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone-macro-parentheses.html


> > All this patch does is to put macro variables in brackets
> > and remove semicolons..
> >
> >> Furthermore I am not really sure whether removing the ';' is even
> >> something worthwhile; they are surely unnecessary (being null
> >> statements), but does this matter?
> >
> > It causes a warning
> >
> >
> https://releases.llvm.org/13.0.0/tools/clang/docs/DiagnosticsReference
> .html#wextra-semi-stmt
> >
> 
> I don't receive this warning despite using Clang 13.0. Do you have -
> Wall
> or -Wextra or something like that enabled?

I'm using ReSharper C++ which is using clang-tidy from clang 13.0
with -Weverything

What settings do you use?

> IMO a better fix for this would be to wrap the macro in a do {} while
> (0) to keep the macro calls function-like.

Isn't that a bit too... hm.. much/ugly?

> Anyway, you should have mentioned in the commit message that your aim
> is to fix this uncommon warning.

Yes, that makes sense.


> > I don't know how others are working, but I use to work in a way
> where
> > such warnings are shown in the editor and in lists in the IDE
> > even without compilation. Now - when you have a code file that
> > generates like 20, 50 or more warnings, it's much harder to spot
> > those warnings that might be really relevant and hinting at a
> mistake,
> > and you might be just too lazy to go through them each time.
> >
> > The clang diagnostics have been helpful in spotting some actual
> > issues in this very file. That's why I consider it worthwhile
> > to also eliminate such "non-issues".
> >
> 
> I also work like that; e.g. my recent ac3.h header patchset was
> inspired
> by clangd not liking cycles in header inclusions ("In included file:
> main file cannot be included recursively when building a preamble").

Yea, I gathered from some of your patches that you must be using
some tooling as well. Would you allow me the question which IDE you 
are using?

Thanks a lot,
softworkz
diff mbox series

Patch

diff --git a/libavformat/asfdec_f.c b/libavformat/asfdec_f.c
index 81a29f99d5..91c3874ac7 100644
--- a/libavformat/asfdec_f.c
+++ b/libavformat/asfdec_f.c
@@ -906,21 +906,21 @@  static int asf_read_header(AVFormatContext *s)
 }
 
 #define DO_2BITS(bits, var, defval)             \
-    switch (bits & 3) {                         \
+    switch ((bits) & 3) {                       \
     case 3:                                     \
-        var = avio_rl32(pb);                    \
+        (var) = avio_rl32(pb);                  \
         rsize += 4;                             \
         break;                                  \
     case 2:                                     \
-        var = avio_rl16(pb);                    \
+        (var) = avio_rl16(pb);                  \
         rsize += 2;                             \
         break;                                  \
     case 1:                                     \
-        var = avio_r8(pb);                      \
+        (var) = avio_r8(pb);                    \
         rsize++;                                \
         break;                                  \
     default:                                    \
-        var = defval;                           \
+        (var) = (defval);                       \
         break;                                  \
     }
 
@@ -1003,9 +1003,9 @@  static int asf_get_packet(AVFormatContext *s, AVIOContext *pb)
     asf->packet_flags    = c;
     asf->packet_property = d;
 
-    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size);
-    DO_2BITS(asf->packet_flags >> 1, padsize, 0); // sequence ignored
-    DO_2BITS(asf->packet_flags >> 3, padsize, 0); // padding length
+    DO_2BITS(asf->packet_flags >> 5, packet_length, s->packet_size)
+    DO_2BITS(asf->packet_flags >> 1, padsize, 0) // sequence ignored
+    DO_2BITS(asf->packet_flags >> 3, padsize, 0) // padding length
 
     // the following checks prevent overflows and infinite loops
     if (!packet_length || packet_length >= (1U << 29)) {
@@ -1066,9 +1066,9 @@  static int asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
     asf->stream_index     = asf->asfid2avid[num & 0x7f];
     asfst                 = &asf->streams[num & 0x7f];
     // sequence should be ignored!
-    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0);
-    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0);
-    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0);
+    DO_2BITS(asf->packet_property >> 4, asf->packet_seq, 0)
+    DO_2BITS(asf->packet_property >> 2, asf->packet_frag_offset, 0)
+    DO_2BITS(asf->packet_property, asf->packet_replic_size, 0)
     av_log(asf, AV_LOG_TRACE, "key:%d stream:%d seq:%d offset:%d replic_size:%d num:%X packet_property %X\n",
             asf->packet_key_frame, asf->stream_index, asf->packet_seq,
             asf->packet_frag_offset, asf->packet_replic_size, num, asf->packet_property);
@@ -1144,7 +1144,7 @@  static int asf_read_frame_header(AVFormatContext *s, AVIOContext *pb)
         return AVERROR_INVALIDDATA;
     }
     if (asf->packet_flags & 0x01) {
-        DO_2BITS(asf->packet_segsizetype >> 6, asf->packet_frag_size, 0); // 0 is illegal
+        DO_2BITS(asf->packet_segsizetype >> 6, asf->packet_frag_size, 0) // 0 is illegal
         if (rsize > asf->packet_size_left) {
             av_log(s, AV_LOG_ERROR, "packet_replic_size is invalid\n");
             return AVERROR_INVALIDDATA;