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

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Sunday, May 15, 2022 8:12 PM
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix
> macro definition and use
> 
> softworkz:
> > From: softworkz <softworkz at hotmail.com>
> >
> > Signed-off-by: softworkz <softworkz at 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