[PATCH 09/10] misc: fix returning void-valued expression warnings

[PATCH 09/10] misc: fix returning void-valued expression warnings

[Harvey Harrison]

kernel/sched_fair.c:845:3: warning: returning void-valued expression
kernel/sched.c:4343:3: warning: returning void-valued expression
security/security.c:897:2: warning: returning void-valued expression
security/security.c:1014:2: warning: returning void-valued expression
security/security.c:1019:2: warning: returning void-valued expression
sound/sound_core.c:410:2: warning: returning void-valued expression
sound/sound_core.c:427:2: warning: returning void-valued expression

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
Peter, I know you don't like this very much, but it would help reduce the
trivial noise somewhat in a sparse build.

 kernel/sched.c      |    6 ++++--
 kernel/sched_fair.c |    6 ++++--
 security/security.c |    6 +++---
 sound/sound_core.c  |    4 ++--
 4 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index e2f7f5a..864daf8 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -4339,8 +4339,10 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
 	struct rq *rq = this_rq();
 	cputime64_t tmp;
 
-	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
-		return account_guest_time(p, cputime);
+	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0)) {
+		account_guest_time(p, cputime);
+		return;
+	}
 
 	p->stime = cputime_add(p->stime, cputime);
 
diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index 89fa32b..0bd575e 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -841,8 +841,10 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
 	 * queued ticks are scheduled to match the slice, so don't bother
 	 * validating it and just reschedule.
 	 */
-	if (queued)
-		return resched_task(rq_of(cfs_rq)->curr);
+	if (queued) {
+		resched_task(rq_of(cfs_rq)->curr);
+		return;
+	}
 	/*
 	 * don't let the period tick interfere with the hrtick preemption
 	 */
diff --git a/security/security.c b/security/security.c
index 59838a9..8073a1a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -894,7 +894,7 @@ EXPORT_SYMBOL(security_secctx_to_secid);
 
 void security_release_secctx(char *secdata, u32 seclen)
 {
-	return security_ops->release_secctx(secdata, seclen);
+	security_ops->release_secctx(secdata, seclen);
 }
 EXPORT_SYMBOL(security_release_secctx);
 
@@ -1011,12 +1011,12 @@ int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
 
 void security_sk_free(struct sock *sk)
 {
-	return security_ops->sk_free_security(sk);
+	security_ops->sk_free_security(sk);
 }
 
 void security_sk_clone(const struct sock *sk, struct sock *newsk)
 {
-	return security_ops->sk_clone_security(sk, newsk);
+	security_ops->sk_clone_security(sk, newsk);
 }
 
 void security_sk_classify_flow(struct sock *sk, struct flowi *fl)
diff --git a/sound/sound_core.c b/sound/sound_core.c
index 46daca1..b2538bf 100644
--- a/sound/sound_core.c
+++ b/sound/sound_core.c
@@ -407,7 +407,7 @@ EXPORT_SYMBOL(unregister_sound_mixer);
 
 void unregister_sound_midi(int unit)
 {
-	return sound_remove_unit(&chains[2], unit);
+	sound_remove_unit(&chains[2], unit);
 }
 
 EXPORT_SYMBOL(unregister_sound_midi);
@@ -424,7 +424,7 @@ EXPORT_SYMBOL(unregister_sound_midi);
 
 void unregister_sound_dsp(int unit)
 {
-	return sound_remove_unit(&chains[3], unit);
+	sound_remove_unit(&chains[3], unit);
 }
 

-- 
1.5.5.1.305.g7c84

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Boaz Harrosh]

On Thu, May 01 2008 at 1:03 +0300, Harvey Harrison <harvey.harrison@gmail.com> wrote:
> kernel/sched_fair.c:845:3: warning: returning void-valued expression
> kernel/sched.c:4343:3: warning: returning void-valued expression
> security/security.c:897:2: warning: returning void-valued expression
> security/security.c:1014:2: warning: returning void-valued expression
> security/security.c:1019:2: warning: returning void-valued expression
> sound/sound_core.c:410:2: warning: returning void-valued expression
> sound/sound_core.c:427:2: warning: returning void-valued expression
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> Peter, I know you don't like this very much, but it would help reduce the
> trivial noise somewhat in a sparse build.
> 
>  kernel/sched.c      |    6 ++++--
>  kernel/sched_fair.c |    6 ++++--
>  security/security.c |    6 +++---
>  sound/sound_core.c  |    4 ++--
>  4 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index e2f7f5a..864daf8 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -4339,8 +4339,10 @@ void account_system_time(struct task_struct *p, int hardirq_offset,
>  	struct rq *rq = this_rq();
>  	cputime64_t tmp;
>  
> -	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0))
> -		return account_guest_time(p, cputime);
> +	if ((p->flags & PF_VCPU) && (irq_count() - hardirq_offset == 0)) {
> +		account_guest_time(p, cputime);
> +		return;
> +	}

I don't know who invented sparse, but I like this form of return.
1 - It saves me the curly brackets and extra return line. But mainly
2 - It is a programing statement that says: "Me here I'm an equivalent 
  to that other call". So if in the future that inner function starts
  to return, say, an error value, with the first style the compiler will
  error. But with the second style the new error return will be silently
  ignored. So these are not equivalent replacements. The former is a much
  stronger bond between the caller and the callie.

>  
>  	p->stime = cputime_add(p->stime, cputime);
>  
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index 89fa32b..0bd575e 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -841,8 +841,10 @@ entity_tick(struct cfs_rq *cfs_rq, struct sched_entity *curr, int queued)
>  	 * queued ticks are scheduled to match the slice, so don't bother
>  	 * validating it and just reschedule.
>  	 */
> -	if (queued)
> -		return resched_task(rq_of(cfs_rq)->curr);
> +	if (queued) {
> +		resched_task(rq_of(cfs_rq)->curr);
> +		return;
> +	}
>  	/*
>  	 * don't let the period tick interfere with the hrtick preemption
>  	 */
> diff --git a/security/security.c b/security/security.c
> index 59838a9..8073a1a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -894,7 +894,7 @@ EXPORT_SYMBOL(security_secctx_to_secid);
>  
>  void security_release_secctx(char *secdata, u32 seclen)
>  {
> -	return security_ops->release_secctx(secdata, seclen);
> +	security_ops->release_secctx(secdata, seclen);
>  }
>  EXPORT_SYMBOL(security_release_secctx);
>  
> @@ -1011,12 +1011,12 @@ int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
>  
>  void security_sk_free(struct sock *sk)
>  {
> -	return security_ops->sk_free_security(sk);
> +	security_ops->sk_free_security(sk);
>  }
>  
>  void security_sk_clone(const struct sock *sk, struct sock *newsk)
>  {
> -	return security_ops->sk_clone_security(sk, newsk);
> +	security_ops->sk_clone_security(sk, newsk);
>  }
>  
>  void security_sk_classify_flow(struct sock *sk, struct flowi *fl)
> diff --git a/sound/sound_core.c b/sound/sound_core.c
> index 46daca1..b2538bf 100644
> --- a/sound/sound_core.c
> +++ b/sound/sound_core.c
> @@ -407,7 +407,7 @@ EXPORT_SYMBOL(unregister_sound_mixer);
>  
>  void unregister_sound_midi(int unit)
>  {
> -	return sound_remove_unit(&chains[2], unit);
> +	sound_remove_unit(&chains[2], unit);
>  }
>  
>  EXPORT_SYMBOL(unregister_sound_midi);
> @@ -424,7 +424,7 @@ EXPORT_SYMBOL(unregister_sound_midi);
>  
>  void unregister_sound_dsp(int unit)
>  {
> -	return sound_remove_unit(&chains[3], unit);
> +	sound_remove_unit(&chains[3], unit);
>  }
>  
> 

Even without the saving in the if case, I would say fix sparse, it is
very very wrong. a return (void)do(); is not equal to do(); return;

Code is not only high level assembly, it is also a low level English.
In English the first form is telling what the future assembly should
be.

big NACK from me (== $0.02)

Boaz

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Al Viro]

On Thu, May 01, 2008 at 02:43:50PM +0300, Boaz Harrosh wrote:

> I don't know who invented sparse, but I like this form of return.
> 1 - It saves me the curly brackets and extra return line. But mainly
> 2 - It is a programing statement that says: "Me here I'm an equivalent 
>   to that other call". So if in the future that inner function starts
>   to return, say, an error value, with the first style the compiler will
>   error. But with the second style the new error return will be silently
>   ignored. So these are not equivalent replacements. The former is a much
>   stronger bond between the caller and the callie.

3.  6.8.6.4(1): A return statement with an expression shall not appear in
a function whose return type is void.

Write in C, please.

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Peter Zijlstra]

On Thu, 2008-05-01 at 13:00 +0100, Al Viro wrote:
> On Thu, May 01, 2008 at 02:43:50PM +0300, Boaz Harrosh wrote:
> 
> > I don't know who invented sparse, but I like this form of return.
> > 1 - It saves me the curly brackets and extra return line. But mainly
> > 2 - It is a programing statement that says: "Me here I'm an equivalent 
> >   to that other call". So if in the future that inner function starts
> >   to return, say, an error value, with the first style the compiler will
> >   error. But with the second style the new error return will be silently
> >   ignored. So these are not equivalent replacements. The former is a much
> >   stronger bond between the caller and the callie.
> 
> 3.  6.8.6.4(1): A return statement with an expression shall not appear in
> a function whose return type is void.
> 
> Write in C, please.

We use GNU99 all over the place, or are you going to clear up all the
statement expressions and such other fancy gnu extensions to the
language as well?

I'm really not seeing why this would be wrong, other than the standard
saying it is, ie. I think the standard got it wrong here.

Harvey can just use -Wno-return-void, or someone can modify sparse to
have that default disabled for STANDARD_GNU[89]9.

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Boaz Harrosh]

On Thu, May 01 2008 at 15:00 +0300, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> On Thu, May 01, 2008 at 02:43:50PM +0300, Boaz Harrosh wrote:
> 
>> I don't know who invented sparse, but I like this form of return.
>> 1 - It saves me the curly brackets and extra return line. But mainly
>> 2 - It is a programing statement that says: "Me here I'm an equivalent 
>>   to that other call". So if in the future that inner function starts
>>   to return, say, an error value, with the first style the compiler will
>>   error. But with the second style the new error return will be silently
>>   ignored. So these are not equivalent replacements. The former is a much
>>   stronger bond between the caller and the callie.
> 
> 3.  6.8.6.4(1): A return statement with an expression shall not appear in
> a function whose return type is void.
> 

Please forgive my ignorance, where is this quote from?

> Write in C, please.

I have used this style for ages. I thought it is C, and the compiler never
complained. You must agree that the two statements are not equivalent. Is it
a bad style? I saw it's merits, perhaps it's just me.

Boaz

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Al Viro]

On Thu, May 01, 2008 at 02:17:06PM +0200, Peter Zijlstra wrote:
> > Write in C, please.
> 
> We use GNU99 all over the place, or are you going to clear up all the
> statement expressions and such other fancy gnu extensions to the
> language as well?

Worthless ones - sure.  Statement expressions are not particulary
nice one, but we can't avoid that shit (note that they are not even
well-defined - you can easily make gcc fall over and die with those).

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[Al Viro]

On Thu, May 01, 2008 at 03:42:14PM +0300, Boaz Harrosh wrote:
> > 3.  6.8.6.4(1): A return statement with an expression shall not appear in
> > a function whose return type is void.
> > 
> 
> Please forgive my ignorance, where is this quote from?

C99.  I don't have C90 in front of me, so I can't give you exact quote from
there, but it's been explicitly banned in C90 as well.

Re: [PATCH 09/10] misc: fix returning void-valued expression warnings

[David Miller]

From: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date: Thu, 01 May 2008 14:17:06 +0200

> On Thu, 2008-05-01 at 13:00 +0100, Al Viro wrote:
> > On Thu, May 01, 2008 at 02:43:50PM +0300, Boaz Harrosh wrote:
> > 
> > > I don't know who invented sparse, but I like this form of return.
> > > 1 - It saves me the curly brackets and extra return line. But mainly
> > > 2 - It is a programing statement that says: "Me here I'm an equivalent 
> > >   to that other call". So if in the future that inner function starts
> > >   to return, say, an error value, with the first style the compiler will
> > >   error. But with the second style the new error return will be silently
> > >   ignored. So these are not equivalent replacements. The former is a much
> > >   stronger bond between the caller and the callie.
> > 
> > 3.  6.8.6.4(1): A return statement with an expression shall not appear in
> > a function whose return type is void.
> > 
> > Write in C, please.
> 
> We use GNU99 all over the place, or are you going to clear up all the
> statement expressions and such other fancy gnu extensions to the
> language as well?
> 
> I'm really not seeing why this would be wrong, other than the standard
> saying it is, ie. I think the standard got it wrong here.
> 
> Harvey can just use -Wno-return-void, or someone can modify sparse to
> have that default disabled for STANDARD_GNU[89]9.

Even Linus thinks this construct is fine and said he had a patch to
make sparse allow it.  This discussion is pointless especially since
GCC has allowed this since basically day one, and it's even self
consistent.