* [PATCH v2] staging: media: sunxi: Add bool cast to value @ 2019-07-22 6:06 Nishka Dasgupta 2019-07-22 11:12 ` Jeremy Sowden 2019-07-25 11:24 ` Hans Verkuil 0 siblings, 2 replies; 9+ messages in thread From: Nishka Dasgupta @ 2019-07-22 6:06 UTC (permalink / raw) To: maxime.ripard, paul.kocialkowski, mchehab, gregkh, wens, linux-media, devel, linux-arm-kernel Cc: Nishka Dasgupta Typecast as bool the return value of cedrus_find_format in cedrus_check_format as the return value of cedrus_check_format is always treated like a boolean value. Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> --- Changes in v2: - Add !! to the returned pointer to ensure that the return value is always either true or false, and never a non-zero value other than true. drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c index e2b530b1a956..b731745f21f8 100644 --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, static bool cedrus_check_format(u32 pixelformat, u32 directions, unsigned int capabilities) { - return cedrus_find_format(pixelformat, directions, capabilities); + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); } static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) -- 2.19.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 6:06 [PATCH v2] staging: media: sunxi: Add bool cast to value Nishka Dasgupta @ 2019-07-22 11:12 ` Jeremy Sowden 2019-07-22 12:24 ` Paul Kocialkowski 2019-07-25 11:24 ` Hans Verkuil 1 sibling, 1 reply; 9+ messages in thread From: Jeremy Sowden @ 2019-07-22 11:12 UTC (permalink / raw) To: Nishka Dasgupta Cc: devel, maxime.ripard, gregkh, paul.kocialkowski, wens, mchehab, linux-arm-kernel, linux-media [-- Attachment #1.1: Type: text/plain, Size: 1563 bytes --] On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > Typecast as bool the return value of cedrus_find_format in > cedrus_check_format as the return value of cedrus_check_format is > always treated like a boolean value. > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > --- > Changes in v2: > - Add !! to the returned pointer to ensure that the return value is > always either true or false, and never a non-zero value other than > true. > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > index e2b530b1a956..b731745f21f8 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > static bool cedrus_check_format(u32 pixelformat, u32 directions, > unsigned int capabilities) > { > - return cedrus_find_format(pixelformat, directions, capabilities); > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > } I think the original was fine. The return value of cedrus_find_format will be automatically converted to bool before being returned from cedrus_check_format since that is the return-type of the function, and the result of converting any non-zero value to bool is 1. > static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > -- > 2.19.1 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 11:12 ` Jeremy Sowden @ 2019-07-22 12:24 ` Paul Kocialkowski 2019-07-22 13:19 ` Jeremy Sowden 2019-07-22 13:44 ` Nishka Dasgupta 0 siblings, 2 replies; 9+ messages in thread From: Paul Kocialkowski @ 2019-07-22 12:24 UTC (permalink / raw) To: Jeremy Sowden Cc: devel, maxime.ripard, gregkh, wens, Nishka Dasgupta, mchehab, linux-arm-kernel, linux-media Hi, On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > > Typecast as bool the return value of cedrus_find_format in > > cedrus_check_format as the return value of cedrus_check_format is > > always treated like a boolean value. > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > > --- > > Changes in v2: > > - Add !! to the returned pointer to ensure that the return value is > > always either true or false, and never a non-zero value other than > > true. > > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > index e2b530b1a956..b731745f21f8 100644 > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > > static bool cedrus_check_format(u32 pixelformat, u32 directions, > > unsigned int capabilities) > > { > > - return cedrus_find_format(pixelformat, directions, capabilities); > > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > > } > > I think the original was fine. The return value of cedrus_find_format > will be automatically converted to bool before being returned from > cedrus_check_format since that is the return-type of the function, and > the result of converting any non-zero value to bool is 1. Okay I was a bit unsure about that and wanted to play it on the safe side without really looking it up, but that gave me the occasion to verify. From what I could find (from my GNU system's /usr/include/unistring/stdbool.h): Limitations of this substitute, when used in a C89 environment: - In C99, casts and automatic conversions to '_Bool' or 'bool' are performed in such a way that every nonzero value gets converted to 'true', and zero gets converted to 'false'. This doesn't work with this substitute. With this substitute, only the values 0 and 1 give the expected result when converted to _Bool' or 'bool'. So since the kernel is built for C89 (unless I'm mistaken), I don't think the compiler provides any guarantee about bool values being converted to 1 when they are non-zero. As a result, I think it's best to be careful. However, I'm not sure I really see what cocinelle was unhappy about. You mentionned single-line functions, but I don't see how that can be a problem. So in the end, I think we should keep the !! and drop the (bool) cast if there's no particular warning about it. What do you think? Cheers, Paul > > static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > > -- > > 2.19.1 -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 12:24 ` Paul Kocialkowski @ 2019-07-22 13:19 ` Jeremy Sowden 2019-07-22 13:44 ` Nishka Dasgupta 1 sibling, 0 replies; 9+ messages in thread From: Jeremy Sowden @ 2019-07-22 13:19 UTC (permalink / raw) To: Paul Kocialkowski Cc: devel, maxime.ripard, gregkh, wens, Nishka Dasgupta, mchehab, linux-arm-kernel, linux-media [-- Attachment #1.1: Type: text/plain, Size: 3528 bytes --] On 2019-07-22, at 14:24:38 +0200, Paul Kocialkowski wrote: > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: > > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > > > Typecast as bool the return value of cedrus_find_format in > > > cedrus_check_format as the return value of cedrus_check_format is > > > always treated like a boolean value. > > > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > > > --- > > > Changes in v2: > > > - Add !! to the returned pointer to ensure that the return value > > > is always either true or false, and never a non-zero value other > > > than true. > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > index e2b530b1a956..b731745f21f8 100644 > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > > > static bool cedrus_check_format(u32 pixelformat, u32 directions, > > > unsigned int capabilities) > > > { > > > - return cedrus_find_format(pixelformat, directions, capabilities); > > > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > > > } > > > > I think the original was fine. The return value of > > cedrus_find_format will be automatically converted to bool before > > being returned from cedrus_check_format since that is the > > return-type of the function, and the result of converting any > > non-zero value to bool is 1. > > Okay I was a bit unsure about that and wanted to play it on the safe > side without really looking it up, but that gave me the occasion to > verify. > > From what I could find (from my GNU system's > /usr/include/unistring/stdbool.h): > > Limitations of this substitute, when used in a C89 environment: > > - In C99, casts and automatic conversions to '_Bool' or 'bool' > are performed in such a way that every nonzero value gets > converted to 'true', and zero gets converted to 'false'. > This doesn't work with this substitute. With this > substitute, only the values 0 and 1 give the expected result > when converted to _Bool' or 'bool'. > > So since the kernel is built for C89 (unless I'm mistaken), I don't > think the compiler provides any guarantee about bool values being > converted to 1 when they are non-zero. Ick. I checked the C99 standard and assumed that GCC would use the same semantics. I've just tested with "gcc-8 -std=gnu89" and it appears to do the right thing: [azazel@ulthar:/space/azazel/tmp] $ cat test.c #include <stdbool.h> #include <stdio.h> int main (void) { char *p = "test"; bool b = p; fprintf (stderr, "%p, %d\n", (void *) p, b); return 0; } [azazel@ulthar:/space/azazel/tmp] $ gcc-8 -O2 -std=gnu89 -Wall -Wextra test.c -o test [azazel@ulthar:/space/azazel/tmp] $ ./test 0x55d984e0e004, 1 > As a result, I think it's best to be careful. Fair enough. > However, I'm not sure I really see what cocinelle was unhappy about. > You mentionned single-line functions, but I don't see how that can be > a problem. > > So in the end, I think we should keep the !! and drop the (bool) cast > if there's no particular warning about it. > > What do you think? Seems sensible. J. [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 12:24 ` Paul Kocialkowski 2019-07-22 13:19 ` Jeremy Sowden @ 2019-07-22 13:44 ` Nishka Dasgupta 2019-07-22 15:05 ` Paul Kocialkowski 2019-07-22 15:58 ` Sakari Ailus 1 sibling, 2 replies; 9+ messages in thread From: Nishka Dasgupta @ 2019-07-22 13:44 UTC (permalink / raw) To: Paul Kocialkowski, Jeremy Sowden Cc: devel, maxime.ripard, gregkh, wens, mchehab, linux-arm-kernel, linux-media On 22/07/19 5:54 PM, Paul Kocialkowski wrote: > Hi, > > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: >> On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: >>> Typecast as bool the return value of cedrus_find_format in >>> cedrus_check_format as the return value of cedrus_check_format is >>> always treated like a boolean value. >>> >>> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> >>> --- >>> Changes in v2: >>> - Add !! to the returned pointer to ensure that the return value is >>> always either true or false, and never a non-zero value other than >>> true. >>> >>> drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>> index e2b530b1a956..b731745f21f8 100644 >>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c >>> @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, >>> static bool cedrus_check_format(u32 pixelformat, u32 directions, >>> unsigned int capabilities) >>> { >>> - return cedrus_find_format(pixelformat, directions, capabilities); >>> + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); >>> } >> >> I think the original was fine. The return value of cedrus_find_format >> will be automatically converted to bool before being returned from >> cedrus_check_format since that is the return-type of the function, and >> the result of converting any non-zero value to bool is 1. > > Okay I was a bit unsure about that and wanted to play it on the safe side > without really looking it up, but that gave me the occasion to verify. > > From what I could find (from my GNU system's /usr/include/unistring/stdbool.h): > > Limitations of this substitute, when used in a C89 environment: > > - In C99, casts and automatic conversions to '_Bool' or 'bool' are > performed in such a way that every nonzero value gets converted > to 'true', and zero gets converted to 'false'. This doesn't work > with this substitute. With this substitute, only the values 0 and 1 > give the expected result when converted to _Bool' or 'bool'. > > So since the kernel is built for C89 (unless I'm mistaken), I don't think the > compiler provides any guarantee about bool values being converted to 1 when > they are non-zero. As a result, I think it's best to be careful. > > However, I'm not sure I really see what cocinelle was unhappy about. You > mentionned single-line functions, but I don't see how that can be a problem. It's not a problem per se. I'm just working on a cleanup project for which I went through all of staging replacing single-line functions with what they were calling. In some cases that makes it easier to figure out what a particular function call does, since the called function actually does something itself instead of just calling a different function? This function was also flagged as one such potentially-removable function by Coccinelle; but in order to do the same replacement that I'd done in other staging drivers, I thought I would do something about the type mismatch first, especially since find_format doesn't appear to be used anywhere else. However, now I won't remove check_format and replace it with find_format as I'd originally planned, since you've said that isn't necessary here. That leaves the return type issue. > So in the end, I think we should keep the !! and drop the (bool) cast if there's > no particular warning about it. Should I send a version 3 that does this? Thanking you, Nishka > What do you think? > > Cheers, > > Paul > >>> static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) >>> -- >>> 2.19.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 13:44 ` Nishka Dasgupta @ 2019-07-22 15:05 ` Paul Kocialkowski 2019-07-22 15:58 ` Sakari Ailus 1 sibling, 0 replies; 9+ messages in thread From: Paul Kocialkowski @ 2019-07-22 15:05 UTC (permalink / raw) To: Nishka Dasgupta Cc: devel, Jeremy Sowden, maxime.ripard, gregkh, wens, mchehab, linux-arm-kernel, linux-media Hi, On Mon 22 Jul 19, 19:14, Nishka Dasgupta wrote: > On 22/07/19 5:54 PM, Paul Kocialkowski wrote: > > Hi, > > > > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: > > > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > > > > Typecast as bool the return value of cedrus_find_format in > > > > cedrus_check_format as the return value of cedrus_check_format is > > > > always treated like a boolean value. > > > > > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > > > > --- > > > > Changes in v2: > > > > - Add !! to the returned pointer to ensure that the return value is > > > > always either true or false, and never a non-zero value other than > > > > true. > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > index e2b530b1a956..b731745f21f8 100644 > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > > > > static bool cedrus_check_format(u32 pixelformat, u32 directions, > > > > unsigned int capabilities) > > > > { > > > > - return cedrus_find_format(pixelformat, directions, capabilities); > > > > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > > > > } > > > > > > I think the original was fine. The return value of cedrus_find_format > > > will be automatically converted to bool before being returned from > > > cedrus_check_format since that is the return-type of the function, and > > > the result of converting any non-zero value to bool is 1. > > > > Okay I was a bit unsure about that and wanted to play it on the safe side > > without really looking it up, but that gave me the occasion to verify. > > > > From what I could find (from my GNU system's /usr/include/unistring/stdbool.h): > > > > Limitations of this substitute, when used in a C89 environment: > > > > - In C99, casts and automatic conversions to '_Bool' or 'bool' are > > performed in such a way that every nonzero value gets converted > > to 'true', and zero gets converted to 'false'. This doesn't work > > with this substitute. With this substitute, only the values 0 and 1 > > give the expected result when converted to _Bool' or 'bool'. > > > > So since the kernel is built for C89 (unless I'm mistaken), I don't think the > > compiler provides any guarantee about bool values being converted to 1 when > > they are non-zero. As a result, I think it's best to be careful. > > > > However, I'm not sure I really see what cocinelle was unhappy about. You > > mentionned single-line functions, but I don't see how that can be a problem. > > It's not a problem per se. I'm just working on a cleanup project for which I > went through all of staging replacing single-line functions with what they > were calling. In some cases that makes it easier to figure out what a > particular function call does, since the called function actually does > something itself instead of just calling a different function? > This function was also flagged as one such potentially-removable function by > Coccinelle; but in order to do the same replacement that I'd done in other > staging drivers, I thought I would do something about the type mismatch > first, especially since find_format doesn't appear to be used anywhere else. > However, now I won't remove check_format and replace it with find_format as > I'd originally planned, since you've said that isn't necessary here. That > leaves the return type issue. Is that an issue reported by coccinelle? If so, could you provide details about it? > > So in the end, I think we should keep the !! and drop the (bool) cast if there's > > no particular warning about it. > > Should I send a version 3 that does this? Yes I think !! would work out fine either way. Apparently we don't need any explicit (bool) cast from a pointer anyway. Cheers, Paul > Thanking you, > Nishka > > > What do you think? > > > > Cheers, > > > > Paul > > > > > > static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > > > > -- > > > > 2.19.1 > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 13:44 ` Nishka Dasgupta 2019-07-22 15:05 ` Paul Kocialkowski @ 2019-07-22 15:58 ` Sakari Ailus 2019-07-23 7:16 ` Paul Kocialkowski 1 sibling, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2019-07-22 15:58 UTC (permalink / raw) To: Nishka Dasgupta Cc: devel, Jeremy Sowden, maxime.ripard, gregkh, Paul Kocialkowski, wens, mchehab, linux-arm-kernel, linux-media On Mon, Jul 22, 2019 at 07:14:08PM +0530, Nishka Dasgupta wrote: > On 22/07/19 5:54 PM, Paul Kocialkowski wrote: > > Hi, > > > > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: > > > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > > > > Typecast as bool the return value of cedrus_find_format in > > > > cedrus_check_format as the return value of cedrus_check_format is > > > > always treated like a boolean value. > > > > > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > > > > --- > > > > Changes in v2: > > > > - Add !! to the returned pointer to ensure that the return value is > > > > always either true or false, and never a non-zero value other than > > > > true. > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > index e2b530b1a956..b731745f21f8 100644 > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > > > > static bool cedrus_check_format(u32 pixelformat, u32 directions, > > > > unsigned int capabilities) > > > > { > > > > - return cedrus_find_format(pixelformat, directions, capabilities); > > > > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > > > > } > > > > > > I think the original was fine. The return value of cedrus_find_format > > > will be automatically converted to bool before being returned from > > > cedrus_check_format since that is the return-type of the function, and > > > the result of converting any non-zero value to bool is 1. > > > > Okay I was a bit unsure about that and wanted to play it on the safe side > > without really looking it up, but that gave me the occasion to verify. > > > > From what I could find (from my GNU system's /usr/include/unistring/stdbool.h): > > > > Limitations of this substitute, when used in a C89 environment: > > > > - In C99, casts and automatic conversions to '_Bool' or 'bool' are > > performed in such a way that every nonzero value gets converted > > to 'true', and zero gets converted to 'false'. This doesn't work > > with this substitute. With this substitute, only the values 0 and 1 > > give the expected result when converted to _Bool' or 'bool'. > > > > So since the kernel is built for C89 (unless I'm mistaken), I don't think the > > compiler provides any guarantee about bool values being converted to 1 when > > they are non-zero. As a result, I think it's best to be careful. > > > > However, I'm not sure I really see what cocinelle was unhappy about. You > > mentionned single-line functions, but I don't see how that can be a problem. > > It's not a problem per se. I'm just working on a cleanup project for which I > went through all of staging replacing single-line functions with what they > were calling. In some cases that makes it easier to figure out what a > particular function call does, since the called function actually does > something itself instead of just calling a different function? > This function was also flagged as one such potentially-removable function by > Coccinelle; but in order to do the same replacement that I'd done in other > staging drivers, I thought I would do something about the type mismatch > first, especially since find_format doesn't appear to be used anywhere else. > However, now I won't remove check_format and replace it with find_format as > I'd originally planned, since you've said that isn't necessary here. That > leaves the return type issue. > > > > So in the end, I think we should keep the !! and drop the (bool) cast if there's > > no particular warning about it. > > Should I send a version 3 that does this? bool was introduced in C99. Converting a non-zero value to boolean will yield true as a result. Please keep the code as-is; it's much easier to read that way. -- Kind regards, Sakari Ailus _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 15:58 ` Sakari Ailus @ 2019-07-23 7:16 ` Paul Kocialkowski 0 siblings, 0 replies; 9+ messages in thread From: Paul Kocialkowski @ 2019-07-23 7:16 UTC (permalink / raw) To: Sakari Ailus Cc: devel, Jeremy Sowden, maxime.ripard, gregkh, wens, Nishka Dasgupta, mchehab, linux-arm-kernel, linux-media Hi, On Mon 22 Jul 19, 18:58, Sakari Ailus wrote: > On Mon, Jul 22, 2019 at 07:14:08PM +0530, Nishka Dasgupta wrote: > > On 22/07/19 5:54 PM, Paul Kocialkowski wrote: > > > Hi, > > > > > > On Mon 22 Jul 19, 12:12, Jeremy Sowden wrote: > > > > On 2019-07-22, at 11:36:51 +0530, Nishka Dasgupta wrote: > > > > > Typecast as bool the return value of cedrus_find_format in > > > > > cedrus_check_format as the return value of cedrus_check_format is > > > > > always treated like a boolean value. > > > > > > > > > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > > > > > --- > > > > > Changes in v2: > > > > > - Add !! to the returned pointer to ensure that the return value is > > > > > always either true or false, and never a non-zero value other than > > > > > true. > > > > > > > > > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > > index e2b530b1a956..b731745f21f8 100644 > > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > > > > > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > > > > > static bool cedrus_check_format(u32 pixelformat, u32 directions, > > > > > unsigned int capabilities) > > > > > { > > > > > - return cedrus_find_format(pixelformat, directions, capabilities); > > > > > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); > > > > > } > > > > > > > > I think the original was fine. The return value of cedrus_find_format > > > > will be automatically converted to bool before being returned from > > > > cedrus_check_format since that is the return-type of the function, and > > > > the result of converting any non-zero value to bool is 1. > > > > > > Okay I was a bit unsure about that and wanted to play it on the safe side > > > without really looking it up, but that gave me the occasion to verify. > > > > > > From what I could find (from my GNU system's /usr/include/unistring/stdbool.h): > > > > > > Limitations of this substitute, when used in a C89 environment: > > > > > > - In C99, casts and automatic conversions to '_Bool' or 'bool' are > > > performed in such a way that every nonzero value gets converted > > > to 'true', and zero gets converted to 'false'. This doesn't work > > > with this substitute. With this substitute, only the values 0 and 1 > > > give the expected result when converted to _Bool' or 'bool'. > > > > > > So since the kernel is built for C89 (unless I'm mistaken), I don't think the > > > compiler provides any guarantee about bool values being converted to 1 when > > > they are non-zero. As a result, I think it's best to be careful. > > > > > > However, I'm not sure I really see what cocinelle was unhappy about. You > > > mentionned single-line functions, but I don't see how that can be a problem. > > > > It's not a problem per se. I'm just working on a cleanup project for which I > > went through all of staging replacing single-line functions with what they > > were calling. In some cases that makes it easier to figure out what a > > particular function call does, since the called function actually does > > something itself instead of just calling a different function? > > This function was also flagged as one such potentially-removable function by > > Coccinelle; but in order to do the same replacement that I'd done in other > > staging drivers, I thought I would do something about the type mismatch > > first, especially since find_format doesn't appear to be used anywhere else. > > However, now I won't remove check_format and replace it with find_format as > > I'd originally planned, since you've said that isn't necessary here. That > > leaves the return type issue. > > > > > > > So in the end, I think we should keep the !! and drop the (bool) cast if there's > > > no particular warning about it. > > > > Should I send a version 3 that does this? > > bool was introduced in C99. Converting a non-zero value to boolean will > yield true as a result. Please keep the code as-is; it's much easier to > read that way. I was quite doubtful about that given the research and conclusions from yesterday, but it seems that Linux is built for GNU 89 (not C89) which brings support for bool "as in C99", so according to what you described. So tl;dr, I agree with you that we should just keep the code as it is now. Cheers, Paul -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value 2019-07-22 6:06 [PATCH v2] staging: media: sunxi: Add bool cast to value Nishka Dasgupta 2019-07-22 11:12 ` Jeremy Sowden @ 2019-07-25 11:24 ` Hans Verkuil 1 sibling, 0 replies; 9+ messages in thread From: Hans Verkuil @ 2019-07-25 11:24 UTC (permalink / raw) To: Nishka Dasgupta, maxime.ripard, paul.kocialkowski, mchehab, gregkh, wens, linux-media, devel, linux-arm-kernel On 7/22/19 8:06 AM, Nishka Dasgupta wrote: > Typecast as bool the return value of cedrus_find_format in > cedrus_check_format as the return value of cedrus_check_format is always > treated like a boolean value. > > Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com> > --- > Changes in v2: > - Add !! to the returned pointer to ensure that the return value is > always either true or false, and never a non-zero value other than > true. > > drivers/staging/media/sunxi/cedrus/cedrus_video.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > index e2b530b1a956..b731745f21f8 100644 > --- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c > @@ -86,7 +86,7 @@ static struct cedrus_format *cedrus_find_format(u32 pixelformat, u32 directions, > static bool cedrus_check_format(u32 pixelformat, u32 directions, > unsigned int capabilities) > { > - return cedrus_find_format(pixelformat, directions, capabilities); > + return !!(bool)cedrus_find_format(pixelformat, directions, capabilities); Why not write: return cedrus_find_format(pixelformat, directions, capabilities) != NULL; That way coccinelle should be happy, and it is clear that cedrus_find_format returns a pointer and that we return true if it is non-NULL. Regards, Hans > } > > static void cedrus_prepare_format(struct v4l2_pix_format *pix_fmt) > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-25 11:24 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-07-22 6:06 [PATCH v2] staging: media: sunxi: Add bool cast to value Nishka Dasgupta 2019-07-22 11:12 ` Jeremy Sowden 2019-07-22 12:24 ` Paul Kocialkowski 2019-07-22 13:19 ` Jeremy Sowden 2019-07-22 13:44 ` Nishka Dasgupta 2019-07-22 15:05 ` Paul Kocialkowski 2019-07-22 15:58 ` Sakari Ailus 2019-07-23 7:16 ` Paul Kocialkowski 2019-07-25 11:24 ` Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).