Date: Sat, 16 Dec 2000 11:11:06 -0700 (MST) From: Nate Williams <nate@yogotech.com> To: Greg Lehey <grog@lemis.com> Cc: Warner Losh <imp@village.org>, chat@FreeBSD.org Subject: Re: Coding style (was Re: cvs commit: [...] pci.c [...]) Message-ID: <14907.45114.931200.484569@nomad.yogotech.com> 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> <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>
next in thread | previous in thread | raw e-mail | index | archive | help
[ 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?14907.45114.931200.484569>