From owner-svn-src-projects@FreeBSD.ORG Fri Jun 19 03:00:28 2009 Return-Path: Delivered-To: svn-src-projects@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 730C51065674; Fri, 19 Jun 2009 03:00:28 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail07.syd.optusnet.com.au (mail07.syd.optusnet.com.au [211.29.132.188]) by mx1.freebsd.org (Postfix) with ESMTP id 0DEE88FC17; Fri, 19 Jun 2009 03:00:27 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-159-184.carlnfd1.nsw.optusnet.com.au (c122-106-159-184.carlnfd1.nsw.optusnet.com.au [122.106.159.184]) by mail07.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id n5J30OSC013677 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 19 Jun 2009 13:00:25 +1000 Date: Fri, 19 Jun 2009 13:00:24 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Brooks Davis In-Reply-To: <20090618150318.GD46033@lor.one-eyed-alien.net> Message-ID: <20090619114256.A28185@delplex.bde.org> References: <200906171940.n5HJerMr086037@svn.freebsd.org> <4A3A10D5.8000601@freebsd.org> <20090618150318.GD46033@lor.one-eyed-alien.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org, Andriy Gapon Subject: Re: svn commit: r194389 - projects/ngroups/sys/kern X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jun 2009 03:00:29 -0000 On Thu, 18 Jun 2009, Brooks Davis wrote: > On Thu, Jun 18, 2009 at 01:03:01PM +0300, Andriy Gapon wrote: >> on 17/06/2009 22:40 Brooks Davis said the following: >>> ... >>> Log: >>> use MIN() instead of min() in one case. >> >> I think 'what' was pretty obvious from the diff, 'why' is what I am curious about :-) >> Just trying to learn something. Well, the use of min() is a type error, while the use of MIN() is just a style bug. Type error: cr_ngroups is signed, at least on -current, so imin() should be used. This type error should be benign, since cr_ngroups should be non- negative and less than INT_MAX no matter what its type is. Style bug: 4.4BSD removed MIN() from in the kernel, and also removed all uses of it in the kernel except in netiso. It was superseded by the imin() family. Thus it should never be used. Similarly in userland, except it hasn't been superseded in userland yet. It was gone from for 10 years until FreeBSD broke this in 2003. In the 10 years, FreeBSD accumulated an enormous amount of cruft to subsede the imin() family. E.g., in FreeBSD-4 /sys there are about 40 private home-made definitions of MIN() with about 140 invocations. Instead of fixing these or the API, FreeBSD subseded the imin() family. API bugs: - MIN() shouldn't exist because it is unnecessarily unsafe. - The imin() family shouldn't exist since it is unnecessarily hard to use. Type errors in its use are normal. It is hard to know whether to use imin() or lmin() for a typedefed type, and while it is easy to determine a type's signedness, this is rarely done. Fixing the API bugs: gcc has only documented how to right a type generic min() (actually max()) for about 20 years using its typeof() extension. See gcc.info. This extension can also be used to write macro versions of the imin() family that either just work like a safe min() would, or detect the type mismatches. Older history: NetBSD subseded imin() in sys/param.h in 2001. Its log message only claims replacing a dozen copies of MIN(). FreeBSD-1 has a much more broken MIN() macro in in the kernel, apparently from preparing to remove MIN() in Net/2. It defines MIN() as imin() in the kernel, where imin() has the same API and an essentially equivalent implementation as the current imin(); thus the only advantage of MIN(), namely its type-genericness, is completely broken (since imin() only works right for signed types no larger than int). The inline version is only in FreeBSD-1 on i386 (in machine/cpufunc.h). There is an unused non-inline version of imin() (presumably from Net/2) in kern/subr_xxx.c. > Because the file already used MIN and all the other cases in the system > that involve some form on ngroups also use MIN. Bug for bug compatibility isn't a good reason :-). The type mismatch version is also used on cr_ngroups in 1 place kern_prot.c, at least in -current on June 13. MIN() is used on cr_ngroups in 2 places in uipc_usrreq.c. This is an old FreeBSD style bug -- it is part of the cruft in FreeBSD-4; uipc_usrreq.c used to define a home-made MIN() just to use it in these 2 places. Current state: kern/*.c on June 13 has 63 lines matching "[^A-Za-z0-9_]min(", only 4 lines matching "[^A-Za-z0-9_]imin(", and 34 lines matching "[^A-Za-z0-9_]MIN(". It seems likely than many of the 63 lines have type errors (I would expect many more signed and/or long types than plain u_int, so I would expect more imin()s than min()s). 34 is 24 more matches for MIN() than in FreeBSD-4. Bruce