[FFmpeg-devel] [PATCH v4 06/10] libavformat/asfdec: fix macro definition and use
Andreas Rheinhardt
andreas.rheinhardt at outlook.com
Mon May 16 11:48:57 EEST 2022
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?
>
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
More information about the ffmpeg-devel
mailing list