diff mbox

[FFmpeg-devel] lavfi/fspp: Add a cast to silence a clang warning

Message ID CAB0OVGrYRQopkuwZ9wESVok9uL_9gthWMc8MKjbDpvg=Rbnc9g@mail.gmail.com
State Superseded
Headers show

Commit Message

Carl Eugen Hoyos April 13, 2019, 10:04 p.m. UTC
Hi!

Attached patch silences two warnings shown when compiling with clang:
libavfilter/vf_fspp.h:51:42: warning: implicit conversion from 'int'
to 'int16_t' (aka 'short') changes value from 44130 to -21406

Please comment, Carl Eugen

Comments

James Almer April 13, 2019, 10:25 p.m. UTC | #1
On 4/13/2019 7:04 PM, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch silences two warnings shown when compiling with clang:
> libavfilter/vf_fspp.h:51:42: warning: implicit conversion from 'int'
> to 'int16_t' (aka 'short') changes value from 44130 to -21406
> 
> Please comment, Carl Eugen
> 
> 
> 0001-lavfi-vf_fspp-Use-the-intended-cast-to-int16_t.patch
> 
> From d731e523d9c5854183d20d37fe921f49fb048498 Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Sat, 13 Apr 2019 23:05:44 +0200
> Subject: [PATCH] lavfi/fspp: Add two casts to int16_t.
> 
> Silences a warning with clang:
> warning: implicit conversion from 'int' to 'int16_t' (aka 'short') changes value from 44130 to -21406
> ---
>  libavfilter/vf_fspp.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavfilter/vf_fspp.h b/libavfilter/vf_fspp.h
> index 74a3447..e346439 100644
> --- a/libavfilter/vf_fspp.h
> +++ b/libavfilter/vf_fspp.h
> @@ -31,7 +31,7 @@
>  #define DCTSIZE 8
>  #define DCTSIZE_S "8"
>  
> -#define FIX(x,s)  ((int) ((x) * (1 << s) + 0.5) & 0xffff)
> +#define FIX(x,s)  ((int16_t)((x) * (1 << s) + 0.5) & (int16_t)0xffff)

The "& 0xffff" is turning -21406 into 44130 since it zeroes all the high
bits, and then that value gets converted back to -21406 when stored into
an int16_t variable.

Change the int cast into int16_t if you want since it doesn't hurt (all
the values are safely within the int16_t range anyway), but remove the
"& 0xffff" altogether instead of casting it, to get rid of this warning.

>  #define C64(x)    ((uint64_t)((x) | (x) << 16)) <<32 | (uint64_t)(x) | (uint64_t)(x) << 16
>  #define FIX64(x,s)  C64(FIX(x,s))
>  
> -- 1.7.10.4
> 
> 
> _______________________________________________
> 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".
>
Michael Niedermayer April 13, 2019, 11:02 p.m. UTC | #2
On Sat, Apr 13, 2019 at 07:25:54PM -0300, James Almer wrote:
> On 4/13/2019 7:04 PM, Carl Eugen Hoyos wrote:
> > Hi!
> > 
> > Attached patch silences two warnings shown when compiling with clang:
> > libavfilter/vf_fspp.h:51:42: warning: implicit conversion from 'int'
> > to 'int16_t' (aka 'short') changes value from 44130 to -21406
> > 
> > Please comment, Carl Eugen
> > 
> > 
> > 0001-lavfi-vf_fspp-Use-the-intended-cast-to-int16_t.patch
> > 
> > From d731e523d9c5854183d20d37fe921f49fb048498 Mon Sep 17 00:00:00 2001
> > From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> > Date: Sat, 13 Apr 2019 23:05:44 +0200
> > Subject: [PATCH] lavfi/fspp: Add two casts to int16_t.
> > 
> > Silences a warning with clang:
> > warning: implicit conversion from 'int' to 'int16_t' (aka 'short') changes value from 44130 to -21406
> > ---
> >  libavfilter/vf_fspp.h |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavfilter/vf_fspp.h b/libavfilter/vf_fspp.h
> > index 74a3447..e346439 100644
> > --- a/libavfilter/vf_fspp.h
> > +++ b/libavfilter/vf_fspp.h
> > @@ -31,7 +31,7 @@
> >  #define DCTSIZE 8
> >  #define DCTSIZE_S "8"
> >  
> > -#define FIX(x,s)  ((int) ((x) * (1 << s) + 0.5) & 0xffff)
> > +#define FIX(x,s)  ((int16_t)((x) * (1 << s) + 0.5) & (int16_t)0xffff)
> 
> The "& 0xffff" is turning -21406 into 44130 since it zeroes all the high
> bits, and then that value gets converted back to -21406 when stored into
> an int16_t variable.
> 
> Change the int cast into int16_t if you want since it doesn't hurt (all
> the values are safely within the int16_t range anyway), but remove the
> "& 0xffff" altogether instead of casting it, to get rid of this warning.
> 
> >  #define C64(x)    ((uint64_t)((x) | (x) << 16)) <<32 | (uint64_t)(x) | (uint64_t)(x) << 16
> >  #define FIX64(x,s)  C64(FIX(x,s))

i think the C64 / FIX64 macros depend on the & 0xFFFF. They are maybe unused
and just referenced in comments but having them not give correct values
could still be confusing. The 0xFFFF if its removed should be added to the
code / comments that refer to it in a way that needs it

[...]
diff mbox

Patch

From d731e523d9c5854183d20d37fe921f49fb048498 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Sat, 13 Apr 2019 23:05:44 +0200
Subject: [PATCH] lavfi/fspp: Add two casts to int16_t.

Silences a warning with clang:
warning: implicit conversion from 'int' to 'int16_t' (aka 'short') changes value from 44130 to -21406
---
 libavfilter/vf_fspp.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavfilter/vf_fspp.h b/libavfilter/vf_fspp.h
index 74a3447..e346439 100644
--- a/libavfilter/vf_fspp.h
+++ b/libavfilter/vf_fspp.h
@@ -31,7 +31,7 @@ 
 #define DCTSIZE 8
 #define DCTSIZE_S "8"
 
-#define FIX(x,s)  ((int) ((x) * (1 << s) + 0.5) & 0xffff)
+#define FIX(x,s)  ((int16_t)((x) * (1 << s) + 0.5) & (int16_t)0xffff)
 #define C64(x)    ((uint64_t)((x) | (x) << 16)) <<32 | (uint64_t)(x) | (uint64_t)(x) << 16
 #define FIX64(x,s)  C64(FIX(x,s))
 
-- 
1.7.10.4