Date: Sun, 9 Sep 2012 02:04:04 +0300 From: Aleksandr Rybalko <ray@ddteam.net> To: Aleksandr Rybalko <ray@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: [PATCH]Re: svn commit: r240119 - head/sys/kern Message-ID: <20120909020404.403b0b6e.ray@ddteam.net> In-Reply-To: <20120908020205.fd7f0c7f.ray@freebsd.org> References: <201209042316.q84NGtuA035023@svn.freebsd.org> <20120905135841.D1053@besplex.bde.org> <20120908020205.fd7f0c7f.ray@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 8 Sep 2012 02:02:05 +0300
Aleksandr Rybalko <ray@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@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@ddteam.net>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120909020404.403b0b6e.ray>