Date: Mon, 16 Aug 2004 01:43:19 -0400 From: Brian Fundakowski Feldman <green@FreeBSD.org> To: John-Mark Gurney <gurney_j@resnet.uoregon.edu> 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: <20040816054318.GF980@green.homeunix.org> In-Reply-To: <20040816052054.GR991@funkthat.com> References: <200408150624.i7F6OhhR074096@repoman.freebsd.org> <20040816014244.GB3026@green.homeunix.org> <20040816015108.GQ991@funkthat.com> <20040816023851.GC3026@green.homeunix.org> <20040816052054.GR991@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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:
> > On Sun, Aug 15, 2004 at 06:51:08PM -0700, John-Mark Gurney wrote:
> > > sure, I'd like a quick peak at the patch though (if it takes me more than
> > > a day, go ahead and commit).
> >
> > I'd very much like you to look it over. I haven't tested it, just made
> > the stylistic changes. If it were more complete (i.e. satisfy bde),
> > every spurious blank line (that is, all of them inside functions which
> > are not between variable declarations and code) would be gone, too, but
> > that can be kind of harsh.
>
> As was pointed out... blank lines at the top of functions w/o variables
> are mandated by style(9)...
The manpage is either wrong or misleading, so either way, I'm waiting
to hear from bde.
> > - * 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.
> > - 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.
> 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.
> 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).
> 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.
--
Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\
<> green@FreeBSD.org \ The Power to Serve! \
Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040816054318.GF980>