From owner-cvs-all@FreeBSD.ORG  Tue Apr  6 17:55:52 2004
Return-Path: <owner-cvs-all@FreeBSD.ORG>
Delivered-To: cvs-all@freebsd.org
Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125])
	by hub.freebsd.org (Postfix) with ESMTP id 6E52516A4CF
	for <cvs-all@FreeBSD.org>; Tue,  6 Apr 2004 17:55:52 -0700 (PDT)
Received: from root.org (root.org [67.118.192.226])
	by mx1.FreeBSD.org (Postfix) with SMTP id 4582043D2D
	for <cvs-all@FreeBSD.org>; Tue,  6 Apr 2004 17:55:52 -0700 (PDT)
	(envelope-from nate@root.org)
Received: (qmail 30635 invoked by uid 1000); 7 Apr 2004 00:54:00 -0000
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>
In-Reply-To: <6.0.1.1.1.20040407004244.03f85e80@imap.sfu.ca>
Message-ID: <20040406174517.F30594@root.org>
References: <20040406230958.C01C616A545@hub.freebsd.org>
	<20040406162703.H30263@root.org>
	<6.0.1.1.1.20040407004244.03f85e80@imap.sfu.ca>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
cc: cvs-src@FreeBSD.org
cc: src-committers@FreeBSD.org
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
X-BeenThere: cvs-all@freebsd.org
X-Mailman-Version: 2.1.1
Precedence: list
List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/cvs-all>
List-Post: <mailto:cvs-all@freebsd.org>
List-Help: <mailto:cvs-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>,
	<mailto:cvs-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Wed, 07 Apr 2004 00:55:52 -0000

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