[PATCH v2] staging: media: sunxi: Add bool cast to value

[PATCH v2] staging: media: sunxi: Add bool cast to value

[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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Jeremy Sowden]

[-- 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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Paul Kocialkowski]

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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Jeremy Sowden]

[-- 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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Nishka Dasgupta]

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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Paul Kocialkowski]

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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Sakari Ailus]

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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Paul Kocialkowski]

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

Re: [PATCH v2] staging: media: sunxi: Add bool cast to value

[Hans Verkuil]

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