* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 13:44 ` Nishka Dasgupta
0 siblings, 0 replies; 27+ 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] 27+ messages in thread* [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 13:44 ` Nishka Dasgupta
0 siblings, 0 replies; 27+ messages in thread
From: Nishka Dasgupta @ 2019-07-22 13:44 UTC (permalink / raw)
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,@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 at 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
>
^ permalink raw reply [flat|nested] 27+ 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
-1 siblings, 0 replies; 27+ messages in thread
From: Paul Kocialkowski @ 2019-07-22 15:05 UTC (permalink / raw)
To: Nishka Dasgupta
Cc: Jeremy Sowden, maxime.ripard, mchehab, gregkh, wens, linux-media,
devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 15:05 ` Paul Kocialkowski
0 siblings, 0 replies; 27+ 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] 27+ messages in thread* [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 15:05 ` Paul Kocialkowski
0 siblings, 0 replies; 27+ messages in thread
From: Paul Kocialkowski @ 2019-07-22 15:05 UTC (permalink / raw)
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,@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 at 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
^ permalink raw reply [flat|nested] 27+ 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:58 ` Sakari Ailus
-1 siblings, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2019-07-22 15:58 UTC (permalink / raw)
To: Nishka Dasgupta
Cc: Paul Kocialkowski, Jeremy Sowden, maxime.ripard, mchehab, gregkh,
wens, linux-media, devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 15:58 ` Sakari Ailus
0 siblings, 0 replies; 27+ 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] 27+ messages in thread* [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-22 15:58 ` Sakari Ailus
0 siblings, 0 replies; 27+ messages in thread
From: Sakari Ailus @ 2019-07-22 15:58 UTC (permalink / raw)
On Mon, Jul 22, 2019@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,@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 at 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
^ permalink raw reply [flat|nested] 27+ 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
-1 siblings, 0 replies; 27+ messages in thread
From: Paul Kocialkowski @ 2019-07-23 7:16 UTC (permalink / raw)
To: Sakari Ailus
Cc: Nishka Dasgupta, Jeremy Sowden, maxime.ripard, mchehab, gregkh,
wens, linux-media, devel, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-23 7:16 ` Paul Kocialkowski
0 siblings, 0 replies; 27+ 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] 27+ messages in thread* [PATCH v2] staging: media: sunxi: Add bool cast to value
@ 2019-07-23 7:16 ` Paul Kocialkowski
0 siblings, 0 replies; 27+ messages in thread
From: Paul Kocialkowski @ 2019-07-23 7:16 UTC (permalink / raw)
Hi,
On Mon 22 Jul 19, 18:58, Sakari Ailus wrote:
> On Mon, Jul 22, 2019@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,@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 at 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
^ permalink raw reply [flat|nested] 27+ messages in thread