Date: Tue, 6 Apr 2004 17:54:00 -0700 (PDT) From: Nate Lawson <nate@root.org> To: Colin Percival <colin.percival@wadham.ox.ac.uk> Cc: cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/kern kern_timeout.c src/sys/sys callout.h src/share/man/man9 timeout.9 Message-ID: <20040406174517.F30594@root.org> In-Reply-To: <6.0.1.1.1.20040407004244.03f85e80@imap.sfu.ca> References: <20040406230958.C01C616A545@hub.freebsd.org> <20040406162703.H30263@root.org> <6.0.1.1.1.20040407004244.03f85e80@imap.sfu.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 7 Apr 2004, Colin Percival wrote: > At 00:32 07/04/2004, Nate Lawson wrote: > >For this one, you can move the comment to above the "if" statement and add > >a blank line before it. It's usually best to comment on the whole block > >above the if statement rather than within it. > > Hmm... those comments are written in the context of the "if" blocks. Does > it really make sense to replace > > if(foo) { > /* Bar */ > ... > > with > > /* If foo, then bar */ > if(foo) { > ... > > I'm generally happy to make style changes, but this seems like a rather > peculiar change to make. Nope, you've already included the check in your text so it can move as-is. Consider this example: + if (wakeup_needed) { + /* + * There might be someone waiting + * for the callout to complete. + */ wakeup_needed being non-zero means there is someone waiting for the callout to complete. So you've translated the check into English already, no need to add redundant "if foo" text. A more informative comment might be: /* Notify any threads waiting for the callout to complete. */ if (wakeup_needed) ... > >Are you > >going to remove that shim at some point? Perhaps a BURN_BRIDGES or > >GONE_IN_6 ifdef would be appropriate for that. > > I think this shim can be removed as soon as any modules which know > about callout_stop have been recompiled; I doubt it will take long > before someone makes a change which requires that to happen. :-) It would be nice if it was gone for 5.3R. Thanks for doing this work. -Nate
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040406174517.F30594>