From owner-cvs-all@FreeBSD.ORG Mon Aug 16 05:43:21 2004 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 35ABD16A4CE; Mon, 16 Aug 2004 05:43:21 +0000 (GMT) Received: from green.homeunix.org (pcp04371970pcs.nrockv01.md.comcast.net [69.140.223.203]) by mx1.FreeBSD.org (Postfix) with ESMTP id 913D843D55; Mon, 16 Aug 2004 05:43:20 +0000 (GMT) (envelope-from green@green.homeunix.org) Received: from green.homeunix.org (green@localhost [127.0.0.1]) by green.homeunix.org (8.13.1/8.13.1) with ESMTP id i7G5hJQw004888; Mon, 16 Aug 2004 01:43:19 -0400 (EDT) (envelope-from green@green.homeunix.org) Received: (from green@localhost) by green.homeunix.org (8.13.1/8.13.1/Submit) id i7G5hJ5o004887; Mon, 16 Aug 2004 01:43:19 -0400 (EDT) (envelope-from green) Date: Mon, 16 Aug 2004 01:43:19 -0400 From: Brian Fundakowski Feldman To: John-Mark Gurney Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040816052054.GR991@funkthat.com> User-Agent: Mutt/1.5.6i cc: cvs-src@FreeBSD.org cc: src-committers@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 ... X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Aug 2004 05:43:21 -0000 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. \,,,,,,,,,,,,,,,,,,,,,,\