Date: Sun, 15 Aug 2004 22:55:56 -0700 From: John-Mark Gurney <gurney_j@resnet.uoregon.edu> To: Brian Fundakowski Feldman <green@FreeBSD.org> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/cam/scsi scsi_target.c src/sys/dev/mii mii.c src/sys/fs/fifofs fifo_vnops.c src/sys/gnu/ext2fs ext2_vnops.c src/sys/kern init_main.c kern_conf.c kern_descrip.c kern_event.c kern_exec.c kern_exit.c kern_fork.c kern_sig.c sys_pipe.c tty.c ... Message-ID: <20040816055556.GS991@funkthat.com> In-Reply-To: <20040816054318.GF980@green.homeunix.org> References: <200408150624.i7F6OhhR074096@repoman.freebsd.org> <20040816014244.GB3026@green.homeunix.org> <20040816015108.GQ991@funkthat.com> <20040816023851.GC3026@green.homeunix.org> <20040816052054.GR991@funkthat.com> <20040816054318.GF980@green.homeunix.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Brian Fundakowski Feldman wrote this message on Mon, Aug 16, 2004 at 01:43 -0400:
> On Sun, Aug 15, 2004 at 10:20:55PM -0700, John-Mark Gurney wrote:
> > Brian Fundakowski Feldman wrote this message on Sun, Aug 15, 2004 at 22:38 -0400:
> > > - * internal flag indicating registration done by kernel
> > > + * Internal flag indicating registration done by kernel
> >
> > Forgot the period...
>
> I dunno about closing phrases as if they're sentences... at least
> everything should be capitalized, though, at the beginning.
Well, you did it for other phrases in the patch...
> > > - if ((kev->flags & EV_ADD) == EV_ADD && kqueue_expand(kq, fops,
> > > - kev->ident, 0) != 0) {
> > > - /* unlock and try again */
> > > - FILEDESC_UNLOCK(fdp);
> > > - fdrop(fp, td);
> > > - fp = NULL;
> > > - error = kqueue_expand(kq, fops, kev->ident, waitok);
> > > - if (error)
> > > - goto done;
> > > - goto findkn;
> > > + if (kev->flags & EV_ADD) {
> > > + error = kqueue_expand(kq, fops, kev->ident, 0);
> > > + if (error) {
> > > + /* Unlock and try again */
> > > + FILEDESC_UNLOCK(fdp);
> > > + fdrop(fp, td);
> > > + fp = NULL;
> > > + if (!waitok)
> > > + goto done;
> > > + error = kqueue_expand(kq, fops, kev->ident, 1);
> > > + if (error)
> > > + goto done;
> > > + goto findkn;
> > > + }
> >
> > This one I think is gross (as in large, excessive)... error is not used
> > from the first call to kqueue_expand, and ends up indenting more code then
> > necessary... I find it easier to read the single if statement, then
> > breaking the logic into two seperate blocks..
>
> You might find it easier, but it takes me at least twice as long to
> understand because it looks so dissimilar to most of the kernel.
To each his own.
> > there was is also a case where you sorted mflag back into the set, you
> > keep the variables COMPLETELY out of order.. putting mflag before fd..
> > at a minimum, the mflag assignment should be moved to the body, and the
> > variables properly sorted...
>
> Just like all the extra blank lines, variable sorting and grouping is a
> HUGE can of worms....
>
> > My personal feeling is that this patch is a bit excesive in the changes,
> > and goes beyond the standard of keeping style changes to a minimum, unless
> > you have additional changes to the source file...
> >
> > I was doing the (var & FLAG) == FLAG to make it easier to read w/ all
> > the flag comparisions I was looking at...
>
> It looks like obfuscation, here. The normal usage of that idiom is to
> mask off a specific bit-subset of a variable to test it against a
> specific bit value, so every time I see that instead of var & FLAG or
> !(var & FLAG), my brain gets derailed right off the fastpath and into
> trying to figure out whether we're dealing with bitmasks or a single
> flag.
And for me, my brain gets derailed when I see that... w/ the logic
above, I just look at the == or !=, and that tells me everything I
need to know...
> > As rwatson pointed out, deleting was misspelled, which you missed in
> > your sweep...
>
> Yeah, I also missed some cases of 'foo|bar' (binary operators need
> spaces, and this rule is very seldom followed well in the kernel).
Hmmm... I thought we had a case where for bitwise it wasn't required
and for logical it was...
> > If you can get bde to sign off on the patch completely (esspecially
> > introducing soo much code churn), and completely test it, then I don't
> > have an objection.
> >
> > NOTE: I did not completely review the patch, I only got about half way
> > or so.
>
> I don't really agree that it introduces code churn. The entire file was
> essentially replaced wholesale, so this is a great point to make it look
> more like the rest of the kernel to aid in others' understanding of it.
As is pointed out... style is guideline, not a contract... Everyone
has his own style... and even style(9) is silent on some of the
issues.. Like the flag issue, the only thing is an example...
So, as I stated, I'll let bde decide to what extent your style patch
applies... Though with how much change it introduced, I'd rather see
it done completely, if at all...
--
John-Mark Gurney Voice: +1 415 225 5579
"All that I will do, has been done, All that I have, has not."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040816055556.GS991>