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 ...

[John-Mark Gurney]

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."