From owner-cvs-src@FreeBSD.ORG Mon Aug 16 05:56:05 2004 Return-Path: Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id BBF6B16A544 for ; Mon, 16 Aug 2004 05:56:04 +0000 (GMT) Received: from mail4.speakeasy.net (mail4.speakeasy.net [216.254.0.204]) by mx1.FreeBSD.org (Postfix) with ESMTP id ACCB643D48 for ; Mon, 16 Aug 2004 05:56:00 +0000 (GMT) (envelope-from jmg@hydrogen.funkthat.com) Received: (qmail 12430 invoked from network); 16 Aug 2004 05:55:57 -0000 Received: from gate.funkthat.com (HELO hydrogen.funkthat.com) ([69.17.45.168]) (envelope-sender ) by mail4.speakeasy.net (qmail-ldap-1.03) with SMTP for ; 16 Aug 2004 05:55:57 -0000 Received: from hydrogen.funkthat.com (iforgf@localhost.funkthat.com [127.0.0.1])i7G5tuuU099189; Sun, 15 Aug 2004 22:55:56 -0700 (PDT) (envelope-from jmg@hydrogen.funkthat.com) Received: (from jmg@localhost) by hydrogen.funkthat.com (8.12.10/8.12.10/Submit) id i7G5tuK1099188; Sun, 15 Aug 2004 22:55:56 -0700 (PDT) Date: Sun, 15 Aug 2004 22:55:56 -0700 From: John-Mark Gurney To: Brian Fundakowski Feldman Message-ID: <20040816055556.GS991@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> <20040816054318.GF980@green.homeunix.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20040816054318.GF980@green.homeunix.org> User-Agent: Mutt/1.4.1i X-Operating-System: FreeBSD 4.2-RELEASE i386 X-PGP-Fingerprint: B7 EC EF F8 AE ED A7 31 96 7A 22 B3 D8 56 36 F4 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html 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-src@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: John-Mark Gurney List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Aug 2004 05:56:05 -0000 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."