Date: Sun, 17 Dec 2000 10:14:26 +1030 From: Greg Lehey <grog@lemis.com> To: Nate Williams <nate@yogotech.com> Cc: Warner Losh <imp@village.org>, chat@FreeBSD.org Subject: Re: Coding style (was Re: cvs commit: [...] pci.c [...]) Message-ID: <20001217101426.N98509@wantadilla.lemis.com> In-Reply-To: <14907.45114.931200.484569@nomad.yogotech.com>; from nate@yogotech.com on Sat, Dec 16, 2000 at 11:11:06AM -0700 References: <15273.976919515@winston.osd.bsdi.com> <200012160006.TAA92008@khavrinen.lcs.mit.edu> <3A3AAB12.EB84759E@cup.hp.com> <marcel@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20001217101426.N98509>