diff mbox series

[FFmpeg-devel] fftools/ffmpeg: Fix declaration-after-statement warning

Message ID AM7PR03MB666067209EF2BCCCB38E819A8F109@AM7PR03MB6660.eurprd03.prod.outlook.com
State Accepted
Commit 3ccfd27f1d0a67114baf6f9afd6353214d041fd7
Headers show
Series [FFmpeg-devel] fftools/ffmpeg: Fix declaration-after-statement warning | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt July 17, 2021, 9:34 p.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
What is actually the reason that we stick to this C90 rule?
Is it because of compability with ancient compilers? (Given that we
already require several C99 features, I doubt that there are compilers
which would fail if we stopped adhering to the
declaration-before-statement rule.) Or is it because it is presumed that
it improves clarity and readability?

 fftools/ffmpeg.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Paul B Mahol July 18, 2021, 6:05 p.m. UTC | #1
LGTM
Andreas Rheinhardt July 18, 2021, 7:23 p.m. UTC | #2
Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> What is actually the reason that we stick to this C90 rule?
> Is it because of compability with ancient compilers? (Given that we
> already require several C99 features, I doubt that there are compilers
> which would fail if we stopped adhering to the
> declaration-before-statement rule.) Or is it because it is presumed that
> it improves clarity and readability?
> 

I pushed this to fix the warning, although I'd like to hear other
peoples' opinion on whether the warning should no longer be enabled.

>  fftools/ffmpeg.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index e0f2fe138f..6f6e002604 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -4234,10 +4234,11 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>          float scale = f->rate_emu ? 1.0 : f->readrate;
>          for (i = 0; i < f->nb_streams; i++) {
>              InputStream *ist = input_streams[f->ist_index + i];
> +            int64_t stream_ts_offset, pts, now;
>              if (!ist->nb_packets) continue;
> -            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> -            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
> +            stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
> +            now = (av_gettime_relative() - ist->start) * scale + stream_ts_offset;
>              if (pts > now)
>                  return AVERROR(EAGAIN);
>          }
>
Gyan Doshi July 19, 2021, 7:06 a.m. UTC | #3
On 2021-07-19 00:53, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> What is actually the reason that we stick to this C90 rule?
>> Is it because of compability with ancient compilers? (Given that we
>> already require several C99 features, I doubt that there are compilers
>> which would fail if we stopped adhering to the
>> declaration-before-statement rule.) Or is it because it is presumed that
>> it improves clarity and readability?
>>
> I pushed this to fix the warning, although I'd like to hear other
> peoples' opinion on whether the warning should no longer be enabled.

Other than those compilers adhering to older standards, is there any 
rationale for it?

Regards,
Gyan

>
>>   fftools/ffmpeg.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index e0f2fe138f..6f6e002604 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -4234,10 +4234,11 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>           float scale = f->rate_emu ? 1.0 : f->readrate;
>>           for (i = 0; i < f->nb_streams; i++) {
>>               InputStream *ist = input_streams[f->ist_index + i];
>> +            int64_t stream_ts_offset, pts, now;
>>               if (!ist->nb_packets) continue;
>> -            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>> -            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
>> +            stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>> +            now = (av_gettime_relative() - ist->start) * scale + stream_ts_offset;
>>               if (pts > now)
>>                   return AVERROR(EAGAIN);
>>           }
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
zhilizhao(赵志立) July 19, 2021, 9:30 a.m. UTC | #4
> On Jul 19, 2021, at 3:23 AM, Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 
> Andreas Rheinhardt:
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> What is actually the reason that we stick to this C90 rule?
>> Is it because of compability with ancient compilers? (Given that we
>> already require several C99 features, I doubt that there are compilers
>> which would fail if we stopped adhering to the
>> declaration-before-statement rule.) Or is it because it is presumed that
>> it improves clarity and readability?
>> 
> 
> I pushed this to fix the warning, although I'd like to hear other
> peoples' opinion on whether the warning should no longer be enabled.

For clang, `-Wdeclaration-after-statement` has no effect with -std=c99. So now it’s
more difficult to detect the declaration-after-statement case, since there is no compiler
warning message. I prefer to disable the warning.


> 
>> fftools/ffmpeg.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index e0f2fe138f..6f6e002604 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -4234,10 +4234,11 @@ static int get_input_packet(InputFile *f, AVPacket **pkt)
>>         float scale = f->rate_emu ? 1.0 : f->readrate;
>>         for (i = 0; i < f->nb_streams; i++) {
>>             InputStream *ist = input_streams[f->ist_index + i];
>> +            int64_t stream_ts_offset, pts, now;
>>             if (!ist->nb_packets) continue;
>> -            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
>> -            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>> -            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
>> +            stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
>> +            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
>> +            now = (av_gettime_relative() - ist->start) * scale + stream_ts_offset;
>>             if (pts > now)
>>                 return AVERROR(EAGAIN);
>>         }
>> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Nicolas George July 19, 2021, 9:33 a.m. UTC | #5
"zhilizhao(赵志立)" (12021-07-19):
> For clang, `-Wdeclaration-after-statement` has no effect with -std=c99.

Looks like a clang bug: you asked for the warning, it should give it.

Regards,
Nicolas George July 19, 2021, 9:40 a.m. UTC | #6
Andreas Rheinhardt (12021-07-18):
> I pushed this to fix the warning, although I'd like to hear other
> peoples' opinion on whether the warning should no longer be enabled.

I personally consider it is good code hygiene to group variable
declarations.

It reduces diff noise when we need to use the same variable earlier. It
makes it easier to spot if we already have the variable we need or if we
are about to redeclare one.

I have used languages where variables could not be declared earlier, I
still use languages where declaring is nor idiomatic at all, I have
tried writing C with on-the-spot declarations and came back from it. It
really makes the code harder to maintain.

Regards,
diff mbox series

Patch

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index e0f2fe138f..6f6e002604 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -4234,10 +4234,11 @@  static int get_input_packet(InputFile *f, AVPacket **pkt)
         float scale = f->rate_emu ? 1.0 : f->readrate;
         for (i = 0; i < f->nb_streams; i++) {
             InputStream *ist = input_streams[f->ist_index + i];
+            int64_t stream_ts_offset, pts, now;
             if (!ist->nb_packets) continue;
-            int64_t stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
-            int64_t pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
-            int64_t now = (av_gettime_relative() - ist->start)*scale + stream_ts_offset;
+            stream_ts_offset = FFMAX(ist->first_dts != AV_NOPTS_VALUE ? ist->first_dts : 0, file_start);
+            pts = av_rescale(ist->dts, 1000000, AV_TIME_BASE);
+            now = (av_gettime_relative() - ist->start) * scale + stream_ts_offset;
             if (pts > now)
                 return AVERROR(EAGAIN);
         }