From owner-freebsd-chat Sat Dec 16 15:44:52 2000 From owner-freebsd-chat@FreeBSD.ORG Sat Dec 16 15:44:44 2000 Return-Path: Delivered-To: freebsd-chat@freebsd.org Received: from wantadilla.lemis.com (wantadilla.lemis.com [192.109.197.80]) by hub.freebsd.org (Postfix) with ESMTP id A735D37B400 for ; Sat, 16 Dec 2000 15:44:40 -0800 (PST) Received: by wantadilla.lemis.com (Postfix, from userid 1004) id 33AE66AB68; Sun, 17 Dec 2000 10:14:26 +1030 (CST) Date: Sun, 17 Dec 2000 10:14:26 +1030 From: Greg Lehey To: Nate Williams Cc: Warner Losh , chat@FreeBSD.org Subject: Re: Coding style (was Re: cvs commit: [...] pci.c [...]) Message-ID: <20001217101426.N98509@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> <14907.45114.931200.484569@nomad.yogotech.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i In-Reply-To: <14907.45114.931200.484569@nomad.yogotech.com>; from nate@yogotech.com on Sat, Dec 16, 2000 at 11:11:06AM -0700 Organization: LEMIS, PO Box 460, Echunga SA 5153, Australia Phone: +61-8-8388-8286 Fax: +61-8-8388-8725 Mobile: +61-418-838-708 WWW-Home-Page: http://www.lemis.com/~grog X-PGP-Fingerprint: 6B 7B C3 8C 61 CD 54 AF 13 24 52 F8 6D A4 95 EF Sender: owner-freebsd-chat@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Saturday, 16 December 2000 at 11:11:06 -0700, Nate Williams wrote: > [ 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). Or to 100 character line widths. Or 120 character line widths. Depends on your window. Years ago I decided to limit my lines to 110 characters so that I could print it on a printer, so that comment is valid, modulo line length. > 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. Superfluous. Not useless. Without it, and without reading checksdstate, you wouldn't know whether checksdstate just confirmed the status, or whether it changed it. >> 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? */ Agreed, not every comment is 100% perfect. But you've put the comment where it interrupts the flow. In my book, that should be reserved for larger comment blocks. >> for (sdno = 0; sdno < m.sdcount; sdno++) >> { > > Yuck, but that's a religious thing.. Agreed. I'm not advocating that the project should change to that style. >> struct sd *sd = &SD [plex->sdnos [sdno]]; >> if (sd->state >= sd_reborn) /* sort of up, */ > > Re-written. Sorry, I don't understand this comment. > /* sort of up, */ >> set_sd_state (sd->sdno, sd_stale, setstate_force); /* make it stale */ > > Useless comment. No, superfluous. >> 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. Agreed. >> 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: >> >> /* 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. Agreed, that makes it more legible. I still find comments on the right better. Greg -- Finger grog@lemis.com for PGP public key See complete headers for address and phone numbers To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-chat" in the body of the message