diff mbox series

[FFmpeg-devel] avcodec/mjpegdec: fix -Wparentheses warning

Message ID tencent_04B8D52E270C389216584D79A02E4DF84F07@qq.com
State New
Headers show
Series [FFmpeg-devel] avcodec/mjpegdec: fix -Wparentheses warning | expand

Checks

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

Commit Message

zhilizhao(赵志立) Sept. 23, 2022, 5:40 p.m. UTC
From: Zhao Zhili <zhilizhao@tencent.com>

Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
---
 libavcodec/mjpegdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steven Liu Sept. 23, 2022, 10:10 a.m. UTC | #1
Zhao Zhili <quinkblack@foxmail.com> 于2022年9月23日周五 17:40写道:
>
> From: Zhao Zhili <zhilizhao@tencent.com>
>
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavcodec/mjpegdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
lgtm
>
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index c594950500..d77c644d3b 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -2866,7 +2866,7 @@ the_end:
>          }
>      }
>
> -    if (e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX)) {
> +    if ((e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX))) {
>          char *value = e->value + strspn(e->value, " \n\t\r"), *endptr;
>          int orientation = strtol(value, &endptr, 0);
>
> --
> 2.25.1
>
> _______________________________________________
> 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".
Rémi Denis-Courmont Sept. 23, 2022, 2:35 p.m. UTC | #2
Le perjantaina 23. syyskuuta 2022, 20.40.30 EEST Zhao Zhili a écrit :
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavcodec/mjpegdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index c594950500..d77c644d3b 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -2866,7 +2866,7 @@ the_end:
>          }
>      }
> 
> -    if (e = av_dict_get(s->exif_metadata, "Orientation", e,
> AV_DICT_IGNORE_SUFFIX)) { +    if ((e = av_dict_get(s->exif_metadata,
> "Orientation", e, AV_DICT_IGNORE_SUFFIX))) { char *value = e->value +
> strspn(e->value, " \n\t\r"), *endptr; int orientation = strtol(value,
> &endptr, 0);

Isn't it easier to read if you break the assignment out, and use the result as 
predicate??
Andreas Rheinhardt Sept. 23, 2022, 3:11 p.m. UTC | #3
Zhao Zhili:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> ---
>  libavcodec/mjpegdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index c594950500..d77c644d3b 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -2866,7 +2866,7 @@ the_end:
>          }
>      }
>  
> -    if (e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX)) {
> +    if ((e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX))) {
>          char *value = e->value + strspn(e->value, " \n\t\r"), *endptr;
>          int orientation = strtol(value, &endptr, 0);
>  

Since when do we care about this type of warning? Line 7087 of configure
adds -Wno-parentheses in case the compiler supports it.

- Andreas
zhilizhao(赵志立) Sept. 23, 2022, 5:43 p.m. UTC | #4
> -----Original Message-----
> From: ffmpeg-devel-bounces@ffmpeg.org <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Andreas Rheinhardt
> Sent: 2022年9月23日 23:11
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/mjpegdec: fix -Wparentheses warning
> 
> Zhao Zhili:
> > From: Zhao Zhili <zhilizhao@tencent.com>
> >
> > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > ---
> >  libavcodec/mjpegdec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > index c594950500..d77c644d3b 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -2866,7 +2866,7 @@ the_end:
> >          }
> >      }
> >
> > -    if (e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX)) {
> > +    if ((e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX))) {
> >          char *value = e->value + strspn(e->value, " \n\t\r"), *endptr;
> >          int orientation = strtol(value, &endptr, 0);
> >
> 
> Since when do we care about this type of warning? Line 7087 of configure
> adds -Wno-parentheses in case the compiler supports it.

OK. Before sending the patch, I have checked git log that there is a commit 3fb32ae2
which fixed a bug catched by -Wparentheses. I didn't notice -Wparentheses has been
disabled explicitly since too much false positive. On the other hand, I'd like to suggest
to avoid such coding style for new code.

Please ignore the patch itself.

> 
> - Andreas
> _______________________________________________
> 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(赵志立) Sept. 23, 2022, 5:46 p.m. UTC | #5
> -----Original Message-----
> From: ffmpeg-devel-bounces@ffmpeg.org <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Rémi Denis-Courmont
> Sent: 2022年9月23日 22:35
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH] avcodec/mjpegdec: fix -Wparentheses warning
> 
> Le perjantaina 23. syyskuuta 2022, 20.40.30 EEST Zhao Zhili a écrit :
> > From: Zhao Zhili <zhilizhao@tencent.com>
> >
> > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > ---
> >  libavcodec/mjpegdec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > index c594950500..d77c644d3b 100644
> > --- a/libavcodec/mjpegdec.c
> > +++ b/libavcodec/mjpegdec.c
> > @@ -2866,7 +2866,7 @@ the_end:
> >          }
> >      }
> >
> > -    if (e = av_dict_get(s->exif_metadata, "Orientation", e,
> > AV_DICT_IGNORE_SUFFIX)) { +    if ((e = av_dict_get(s->exif_metadata,
> > "Orientation", e, AV_DICT_IGNORE_SUFFIX))) { char *value = e->value +
> > strspn(e->value, " \n\t\r"), *endptr; int orientation = strtol(value,
> > &endptr, 0);
> 
> Isn't it easier to read if you break the assignment out, and use the result as
> predicate??

I prefer the coding style of splitting into two lines personally, but current coding
Style is used everywhere in the code base, so I'm not sure.

Since -Wparentheses has been disabled explicitly, the patch doesn't matter now.

> 
> --
> レミ・デニ-クールモン
> http://www.remlab.net/
> 
> 
> 
> _______________________________________________
> 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".
Anton Khirnov Sept. 28, 2022, 2:45 p.m. UTC | #6
Quoting Zhao Zhili (2022-09-23 19:46:24)
> 
> 
> > -----Original Message-----
> > From: ffmpeg-devel-bounces@ffmpeg.org <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Rémi Denis-Courmont
> > Sent: 2022年9月23日 22:35
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH] avcodec/mjpegdec: fix -Wparentheses warning
> > 
> > Le perjantaina 23. syyskuuta 2022, 20.40.30 EEST Zhao Zhili a écrit :
> > > From: Zhao Zhili <zhilizhao@tencent.com>
> > >
> > > Signed-off-by: Zhao Zhili <zhilizhao@tencent.com>
> > > ---
> > >  libavcodec/mjpegdec.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> > > index c594950500..d77c644d3b 100644
> > > --- a/libavcodec/mjpegdec.c
> > > +++ b/libavcodec/mjpegdec.c
> > > @@ -2866,7 +2866,7 @@ the_end:
> > >          }
> > >      }
> > >
> > > -    if (e = av_dict_get(s->exif_metadata, "Orientation", e,
> > > AV_DICT_IGNORE_SUFFIX)) { +    if ((e = av_dict_get(s->exif_metadata,
> > > "Orientation", e, AV_DICT_IGNORE_SUFFIX))) { char *value = e->value +
> > > strspn(e->value, " \n\t\r"), *endptr; int orientation = strtol(value,
> > > &endptr, 0);
> > 
> > Isn't it easier to read if you break the assignment out, and use the result as
> > predicate??
> 
> I prefer the coding style of splitting into two lines personally, but current coding
> Style is used everywhere in the code base, so I'm not sure.

Not everywhere, it depends on the author.
I try to avoid assignments in conditions wherever possible, because they
are hard to read IMO.
diff mbox series

Patch

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index c594950500..d77c644d3b 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -2866,7 +2866,7 @@  the_end:
         }
     }
 
-    if (e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX)) {
+    if ((e = av_dict_get(s->exif_metadata, "Orientation", e, AV_DICT_IGNORE_SUFFIX))) {
         char *value = e->value + strspn(e->value, " \n\t\r"), *endptr;
         int orientation = strtol(value, &endptr, 0);