From owner-freebsd-chat Sat Dec 16 10:11:23 2000 From owner-freebsd-chat@FreeBSD.ORG Sat Dec 16 10:11:10 2000 Return-Path: Delivered-To: freebsd-chat@freebsd.org Received: from ns.yogotech.com (ns.yogotech.com [206.127.123.66]) by hub.freebsd.org (Postfix) with ESMTP id E06EA37B400; Sat, 16 Dec 2000 10:11:09 -0800 (PST) Received: from nomad.yogotech.com (nomad.yogotech.com [206.127.123.131]) by ns.yogotech.com (8.9.3/8.9.3) with ESMTP id LAA22993; Sat, 16 Dec 2000 11:11:08 -0700 (MST) (envelope-from nate@nomad.yogotech.com) Received: (from nate@localhost) by nomad.yogotech.com (8.8.8/8.8.8) id LAA05322; Sat, 16 Dec 2000 11:11:07 -0700 (MST) (envelope-from nate) From: Nate Williams MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <14907.45114.931200.484569@nomad.yogotech.com> Date: Sat, 16 Dec 2000 11:11:06 -0700 (MST) To: Greg Lehey Cc: Warner Losh , chat@FreeBSD.org Subject: Re: Coding style (was Re: cvs commit: [...] pci.c [...]) In-Reply-To: <20001216184937.G97408@wantadilla.lemis.com> References: <15273.976919515@winston.osd.bsdi.com> <200012160006.TAA92008@khavrinen.lcs.mit.edu> <3A3AAB12.EB84759E@cup.hp.com> <3A3A96E0.DDDDFA55@cup.hp.com> <14906.41033.930780.802740@nomad.yogotech.com> <20001216114710.H91832@wantadilla.lemis.com> <200012160553.WAA74580@harmony.village.org> <20001216184937.G97408@wantadilla.lemis.com> X-Mailer: VM 6.75 under 21.1 (patch 12) "Channel Islands" XEmacs Lucid Reply-To: nate@yogotech.com (Nate Williams) Sender: owner-freebsd-chat@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org [ Moving to -chat ] > >> I'm not suggesting we reform all of these problems, but the first one > >> certainly needs attention. I'd go along with Marcel's second > >> proposal: > > > > I'm not sure that the first one needs attention. I find the code very > > readable as it stands right now. > > What's there is readable. That doesn't make it intelligible. I've > just spent the best part of a week fighting my way through the pccard > and newbus code trying to work out why my Aviator cards didn't work. > UTSL is a good idea if you don't have anything else, but comments > would be good too. > > Take again my RAID-5 code as an example, both of comments and of the > necessary deviation they cause from style(9). Here's a little of the > code: [ Snip ] Yuck, I can hardly read it. *SERIOUSLY* It's overflows my terminal window so badly that it's useless, and I couldn't even print it on a printer so I can read it (yet another good reason to stick to 80-char line-widths). Tab spaces are less of an issue for me, but 80-char lines are a *BIG* deal in my book. And, useless comments are annoying as well, since if you write good method names and use descriptive variables, it's mostly self documenting. > > if (sd->state != sd_up) > { > enum requeststatus s; > > s = checksdstate (sd, rq, *diskaddr, diskend); /* do we need to change state? */ Useless comment. > if (s && (m.badsdno >= 0)) /* second bad disk, */ This line I would write like this. The 'question' mark implies that it's a question, and the comma has no business being there. This also makes it easier to print and read on every terminal used by man. /* second bad disk? */ > if (s && (m.badsdno >= 0)) > { > int sdno; > /* > * If the parity disk is down, there's > * no recovery. We make all involved > * subdisks stale. Otherwise, we > * should be able to recover, but it's > * like pulling teeth. Fix it later. > */ [ Nice ] > for (sdno = 0; sdno < m.sdcount; sdno++) > { Yuck, but that's a religious thing.. > struct sd *sd = &SD [plex->sdnos [sdno]]; > if (sd->state >= sd_reborn) /* sort of up, */ Re-written. /* sort of up, */ > set_sd_state (sd->sdno, sd_stale, setstate_force); /* make it stale */ Useless comment. etc.... > To save you counting, the first line is 91 characters long. This is > nothing like style(9), of course, so before I commit, I indent(1). > With eight character indents, the result would look like: Then change your comment style so that it conforms better *before* you turn indent. That's really your biggest problem here is the 'end-of-line' comments (which I find extremely annoying, because you have to hunt for them, rather than having them be right where my eyes follow). But, this is religious war, hence the move to -chat. > This would be almost OK except for those damned comments, which make > the fourth line 99 characters long. Of course, I could inline the > comments and break that one remaining code line which is 81 characters > long: > > if (sd->state != sd_up) { > enum requeststatus s; > > /* do we need to change state? */ > s = checksdstate(sd, rq, *diskaddr, diskend); > /* second bad disk, */ > if (s && (m.badsdno >= 0)) { > int sdno; > /* > * If the parity disk is down, there's > * no recovery. We make all involved > * subdisks stale. Otherwise, we > * should be able to recover, but it's > * like pulling teeth. Fix it later. > */ > for (sdno = 0; sdno < m.sdcount; sdno++) { > struct sd *sd = &SD[plex->sdnos[sdno]]; > /* sort of up, */ > if (sd->state >= sd_reborn) > /* make it stale */ > set_sd_state(sd->sdno, > sd_stale, setstate_force); > } > /* and crap out */ > return s; > } > /* note which one is bad */ > m.badsdno = mysdno; > /* we need recovery */ > m.flags |= XFR_DEGRADED_WRITE; > /* count another one */ > plex->degraded_writes++; > /* define the bounds */ > m.groupoffset = m.dataoffset; > m.grouplen = m.datalen; > } else { > > Does that really look better? To me it does, although I add in a few blank lines to break things up into 'logical chunks'. ie; /* note which one is bad */ m.badsdno = mysdno; /* we need recovery */ m.flags |= XFR_DEGRADED_WRITE; /* count another one */ plex->degraded_writes++; /* define the bounds */ m.groupoffset = m.dataoffset; m.grouplen = m.datalen; Much better! Too much white-space is bad, but so is not enough white-space. With snuggly braces you lose alot of white-space, so you can add in white-space back at good places. Nate To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-chat" in the body of the message