| 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 |
| 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 |
LGTM
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); > } >
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".
> 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". >
"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,
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 --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); }
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(-)