[PATCH]Re: svn commit: r240119 - head/sys/kern

[Aleksandr Rybalko]

On Sat, 8 Sep 2012 02:02:05 +0300
Aleksandr Rybalko <ray at freebsd.org> wrote:

> Hi Bruce!
> 
> Did not absorb all details of style(9) yet. But I'm working on that :)
> 
> On Wed, 5 Sep 2012 14:18:40 +1000 (EST)
> Bruce Evans <brde at optusnet.com.au> wrote:
> 
> > On Tue, 4 Sep 2012, Aleksandr Rybalko wrote:
> > 
> > > Log:
> > >  Style fixes.
> > >
> > >  Suggested by:   mdf
> > >  Approved by:	adrian (menthor)
> > 
> > The following style bugs remain.  (The density of style bugs is low
> > enough for them to be easy to fix.)
> > 
> > > Modified: head/sys/kern/subr_hints.c
> > > ==============================================================================
> > > --- head/sys/kern/subr_hints.c	Tue Sep  4 23:13:24
> > > 2012	(r240118) +++ head/sys/kern/subr_hints.c	Tue
> > > Sep  4 23:16:55 2012	(r240119) @@ -31,8 +31,8 @@ __FBSDID
> > > ("$FreeBSD$");
> > > #include <sys/lock.h>
> > > #include <sys/malloc.h>
> > > #include <sys/mutex.h>
> > > -#include <sys/systm.h>
> > > #include <sys/sysctl.h>
> > > +#include <sys/systm.h>
> > 
> > Sorting this correctly would be an unrelated fix (it is a
> > prerequisite for most headers, since almost any header may use
> > KASSERT()).
> 
> Yeah, now I found right place for it.
> 
> > 
> > > #include <sys/bus.h>
> > 
> > Sorting this correctly woruld be an unrelated fix.
> > 
> 
> Yeah, kind of second <sys/types.h> for kernel.
> 
> > >
> > > /*
> > > @@ -52,9 +52,9 @@ static char *hintp;
> > 
> > Sorting and indenting the static variables would be an unrelated
> > fix.
> 
> I swear, I was not touch hintp.  Same with checkmethod and
> use_kenv. :)
> 
> > 
> > > static int
> > > sysctl_hintmode(SYSCTL_HANDLER_ARGS)
> > 
> > A bug in svn diff is visible.  The variable declaration is worse
> > than useless as a header for this block of code.
> > 
> 
> Did not get it.  About which variable you saying?
> 
> > 
> > > {
> > > -	int error, i, from_kenv, value, eqidx;
> > > 	const char *cp;
> > > 	char *line, *eq;
> > > +	int eqidx, error, from_kenv, i, value;
> > >
> > > 	from_kenv = 0;
> > > 	cp = kern_envp;
> > > @@ -62,7 +62,7 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS)
> > >
> > > 	/* Fetch candidate for new hintmode value */
> > 
> > Comments (except possibly ones at the right of code) should be real
> > sentences.  This one is missing a ".", unlike all older comments
> > (not at the right of code) in this file.
> 
> Fixed.
> 
> > 
> > > 	error = sysctl_handle_int(oidp, &value, 0, req);
> > > -	if (error || !req->newptr)
> > > +	if (error || req->newptr == NULL)
> > > 		return (error);
> > >
> > > 	if (value != 2)
> > 
> > This still has a boolean test for the non-boolean `error'.  Now the
> > older code sets a bad example in all cases where `error' is tested.
> 
> Fixed.
> 
> > 
> > > @@ -73,8 +73,11 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS)
> > > 	switch (hintmode) {
> > > 	case 0:
> > > 		if (dynamic_kenv) {
> > > -			/* Already here */
> > > -			hintmode = value; /* XXX: Need we switch
> > > or not ? */
> > > +			/*
> > > +			 * Already here. But assign hintmode to
> > > 2, to not
> > > +			 * check it in the future.
> > > +			 */
> > 
> > Sentence breaks should be 2 spaces, as in all older comments in this
> > file, starting as usual with the copyright.  But outside of the
> > copyright, the style bug of single-space sentence breaks was avoided
> > in this file mostly by using the larger style bug of using a new
> > line for most new sentences.
> 
> Already start delimiting sentences with double space.  When folks last
> time arguing about it, I found power to read only about 10 first
> mails. :)
> 
> > 
> > > +			hintmode = 2;
> > > 			return (0);
> > > 		}
> > > 		from_kenv = 1;
> > > @@ -98,7 +101,7 @@ sysctl_hintmode(SYSCTL_HANDLER_ARGS)
> > > 				continue;
> > > 		}
> > > 		eq = strchr(cp, '=');
> > > -		if (!eq)
> > > +		if (eq == NULL)
> > > 			/* Bad hint value */
> > > 			continue;
> > > 		eqidx = eq - cp;
> > 
> > Bruce
> 
> Thank you very much Bruce!  I am understand how much important is
> code style, but still on the way to that point.
> 

[skip old patch]

http://people.freebsd.org/~ray/subr_hints_style.patch
+1 update by mdf@ hint

Many Thanks!

P.S. Tested - WORKS :)

WBW
-- 
Aleksandr Rybalko <ray at ddteam.net>