Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1592868 > unrolled thread

[PATCH] x86/fpu: fix boolreturn.cocci warnings

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] x86/fpu: fix boolreturn.cocci warnings kbuild test robot <fengguang.wu@intel.com> - 2017-03-06 01:50 +0100
    Re: [PATCH] x86/fpu: fix boolreturn.cocci warnings Ingo Molnar <mingo@kernel.org> - 2017-03-07 08:40 +0100
      Re: [PATCH] x86/fpu: fix boolreturn.cocci warnings Thomas Gleixner <tglx@linutronix.de> - 2017-03-07 09:40 +0100
        Re: [PATCH] x86/fpu: fix boolreturn.cocci warnings Ingo Molnar <mingo@kernel.org> - 2017-03-07 12:00 +0100
          Re: [PATCH] x86/fpu: fix boolreturn.cocci warnings Joe Perches <joe@perches.com> - 2017-03-07 13:10 +0100

#1592868 — [PATCH] x86/fpu: fix boolreturn.cocci warnings

arch/x86/kernel/fpu/xstate.c:931:9-10: WARNING: return of 0/1 in function 'xfeatures_mxcsr_quirk' with return type bool

 Return statements in functions returning bool should use
 true/false instead of 1/0.
Generated by: scripts/coccinelle/misc/boolreturn.cocci

CC: Rik van Riel <riel@redhat.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 xstate.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -928,12 +928,12 @@ int arch_set_user_pkey_access(struct tas
 static inline bool xfeatures_mxcsr_quirk(u64 xfeatures)
 {
 	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
-		return 0;
+		return false;
 
 	if (xfeatures & XFEATURE_MASK_FP)
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 /*

[toc] | [next] | [standalone]

#1593940

* kbuild test robot <fengguang.wu@intel.com> wrote:

> arch/x86/kernel/fpu/xstate.c:931:9-10: WARNING: return of 0/1 in function 'xfeatures_mxcsr_quirk' with return type bool
> 
>  Return statements in functions returning bool should use
>  true/false instead of 1/0.

Note that this is a totally bogus warning. I personally find a 0/1 return more 
readable than a textual 'true/false', even if bools are used, and nowhere does the 
kernel mandate the use of 0/1.

So NAK ...

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1593984

On Tue, 7 Mar 2017, Ingo Molnar wrote:
> 
> * kbuild test robot <fengguang.wu@intel.com> wrote:
> 
> > arch/x86/kernel/fpu/xstate.c:931:9-10: WARNING: return of 0/1 in function 'xfeatures_mxcsr_quirk' with return type bool
> > 
> >  Return statements in functions returning bool should use
> >  true/false instead of 1/0.
> 
> Note that this is a totally bogus warning. I personally find a 0/1 return more 
> readable than a textual 'true/false', even if bools are used, and nowhere does the 
> kernel mandate the use of 0/1.

I disagree.

The fact that booleans have been brought retroactively into the C-Standard
does and for compability reasons C still follows the approach "Boolean
values are just integers" does not make it any better.

We had stupid bugs, where people returned -EINVAL from a boolean function
and introduced silly and hard to understand bugs.

The canonical values assigned to booleans are 'true' and 'false' and not
whatever people prefer. Can we please be consistent on that?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1594110

(Linus and Andrew Cc:-ed)

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Tue, 7 Mar 2017, Ingo Molnar wrote:
> > 
> > * kbuild test robot <fengguang.wu@intel.com> wrote:
> > 
> > > arch/x86/kernel/fpu/xstate.c:931:9-10: WARNING: return of 0/1 in function 'xfeatures_mxcsr_quirk' with return type bool
> > > 
> > >  Return statements in functions returning bool should use
> > >  true/false instead of 1/0.
> > 
> > Note that this is a totally bogus warning. I personally find a 0/1 return more 
> > readable than a textual 'true/false', even if bools are used, and nowhere does the 
> > kernel mandate the use of 0/1.
> 
> I disagree.
> 
> The fact that booleans have been brought retroactively into the C-Standard
> does and for compability reasons C still follows the approach "Boolean
> values are just integers" does not make it any better.
> 
> We had stupid bugs, where people returned -EINVAL from a boolean function
> and introduced silly and hard to understand bugs.

But this function is not using -EINVAL, it's using 0 and 1 which is both correct 
and unambiguous!

I mean, if the Cocci script warned about -EINVAL then it would have found a clear 
bug. Now it's warning about the use of 0/1 literals with bool types which is 
perfectly legal, readable, clear C code!

> The canonical values assigned to booleans are 'true' and 'false' and not 
> whatever people prefer. Can we please be consistent on that?

I think that's backwards, because 1/0 is just as canonical for true/false, and to 
me personally it's in fact easier to read as well.

I would really like higher level buy-in for that principle (I've Cc:-ed Linus and 
Andrew), and if indeed the consensus is that '0/1' cannot be used with 'bool' then 
I'll remove all uses of 'bool' from my patches and from code I care about and use 
'int' instead. Please update Documentation/CodingStyle accordingly as well.

To me a lexical 'true/false' instead of '1/0' is a step backwards in readability 
in many cases - using the slightly wider 'int' type is the lesser evil.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1594159

On Tue, 2017-03-07 at 10:01 +0100, Ingo Molnar wrote:
> To me a lexical 'true/false' instead of '1/0' is a step backwards in readability 
> in many cases

What cases are those to you?

I can't think of any case where 1/0 for true/false
is "more readable" for boolean returns to me.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web