Skip to contentSkip to conversation

sched: add statics, don't return void expressions

PATCH In review Harvey Harrison 3 participants 6 messages Apr 25, 2008

lore
Share & tools
lore

Harvey Harrison

author
#0 lore raw

PATCH 1/1 sched: add statics, don't return void expressions

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static? kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static? kernel/sched_fair.c:845:3: warning: returning void-valued expression kernel/sched.c:4386:3: warning: returning void-valued expression

Signed-off-by Harvey Harrison <[email protected]>
---
 kernel/sched.c      |   10 ++++++----
 kernel/sched_fair.c |    6 ++++--
 2 files changed, 10 insertions(+), 6 deletions(-)
kernel/sched.c +6 -4
index 0014b03..81ff960 100644
@@ -757,14 +757,14 @@ const_debug unsigned int sysctl_sched_features =
 #define SCHED_FEAT(name, enabled)	\
 	#name ,
 
-__read_mostly char *sched_feat_names[] = {
+static __read_mostly char *sched_feat_names[] = {
 #include "sched_features.h"
 	NULL
 };
 
 #undef SCHED_FEAT
 
-int sched_feat_open(struct inode *inode, struct file *filp)
+static int sched_feat_open(struct inode *inode, struct file *filp)
 {
 	filp->private_data = inode->i_private;
 	return 0;

@@ -4382,8 +4382,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);
kernel/sched_fair.c +4 -2
index 89fa32b..0bd575e 100644
@@ -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
 	 */
Signature · 1 line
1.5.5.1.270.g89765
#1 lore raw
Quoted 5 lines · Harvey Harrison

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static? kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static? kernel/sched_fair.c:845:3: warning: returning void-valued expression kernel/sched.c:4386:3: warning: returning void-valued expression

I'm still of two minds about that latter warning, I think:

void foo(void);

void bar(void)

{
	return foo();
}

isn't wrong, as the return types match.

#2 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Harvey Harrison

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static? kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static? kernel/sched_fair.c:845:3: warning: returning void-valued expression kernel/sched.c:4386:3: warning: returning void-valued expression

I'm still of two minds about that latter warning, I think:

Quoted 8 lines

void foo(void);

void bar(void)

{
	return foo();
}

isn't wrong, as the return types match.

But you could save a keyword and use

void bar(void)

{
	foo();
}
#3 lore raw
Quoted 17 lines · Jan Engelhardt
Quoted 16 lines · Peter Zijlstra
Quoted 5 lines · Harvey Harrison

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static? kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static? kernel/sched_fair.c:845:3: warning: returning void-valued expression kernel/sched.c:4386:3: warning: returning void-valued expression

I'm still of two minds about that latter warning, I think:

void foo(void);

void bar(void)

{
	return foo();
}
Quoted 8 lines
Quoted 1 line

isn't wrong, as the return types match.

But you could save a keyword and use

void bar(void)

{
	foo();
}

Sure, but this was just to show the idiom at hand; an actual use case would be something like this:

void bar(void)

{
	if (cond)
		return foo();

	/* do stuff ourselves */
}

Leaving out the return here does have side-effects.

#4 lore raw
Quoted 5 lines · Peter Zijlstra

Sure, but this was just to show the idiom at hand; an actual use case would be something like this:

void bar(void)

Quoted 9 lines
{
	if (cond)
		return foo();

	/* do stuff ourselves */
}

Leaving out the return here does have side-effects.

Of course. You do not get that luxury in the middle of the function :^)

author
#5 lore raw
In reply to Peter Zijlstra
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Harvey Harrison

Noticed by sparse:
kernel/sched.c:760:20: warning: symbol 'sched_feat_names' was not declared. Should it be static? kernel/sched.c:767:5: warning: symbol 'sched_feat_open' was not declared. Should it be static? kernel/sched_fair.c:845:3: warning: returning void-valued expression kernel/sched.c:4386:3: warning: returning void-valued expression

I'm still of two minds about that latter warning, I think:

Quoted 9 lines

void foo(void);

void bar(void)

{
	return foo();
}

isn't wrong, as the return types match.

True it's not wrong, but it is a trivial way to reduce the sparse noise somewhat which can make it harder to see real bugs. Just like the integer used as NULL pointer warnings.

And even though in this case, I can't see the return type changing to actually return something, coding it this way makes it obvious if you ever missed a return site.

No strong feelings.

Harvey