Date: Fri, 19 Jun 2009 13:00:24 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Brooks Davis <brooks@FreeBSD.org> Cc: svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org, Andriy Gapon <avg@FreeBSD.org> Subject: Re: svn commit: r194389 - projects/ngroups/sys/kern Message-ID: <20090619114256.A28185@delplex.bde.org> In-Reply-To: <20090618150318.GD46033@lor.one-eyed-alien.net> References: <200906171940.n5HJerMr086037@svn.freebsd.org> <4A3A10D5.8000601@freebsd.org> <20090618150318.GD46033@lor.one-eyed-alien.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/param.h> 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 <sys/param.h> 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 <sys/param.h> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090619114256.A28185>