Date: Wed, 22 Feb 2017 00:46:31 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Conrad Meyer <cem@freebsd.org> Cc: "Pedro F. Giffuni" <pfg@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r313982 - in head/sys/dev: agp al_eth an arcmsr bce beri/virtio bhnd/cores/usb buslogic ce cm cp ctau cx de ed fatm fe firewire hptiop hptmv iicbus isp le md ncr netmap ofw patm pccard ... Message-ID: <20170221234517.L3157@besplex.bde.org> In-Reply-To: <CAG6CVpUSxaQkq%2B7siKQPOW4FuaVpoY5uMcSb6jMvrROqSiqzGA@mail.gmail.com> References: <201702200343.v1K3hCk3060716@repo.freebsd.org> <CAG6CVpUSxaQkq%2B7siKQPOW4FuaVpoY5uMcSb6jMvrROqSiqzGA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 20 Feb 2017, Conrad Meyer wrote:
> On Sun, Feb 19, 2017 at 7:43 PM, Pedro F. Giffuni <pfg@freebsd.org> wrote:
>> ...
>> Log:
>> sys/dev: Replace zero with NULL for pointers.
>>
>> Makes things easier to read, plus architectures may set NULL to something
>> different than zero.
> ...
> I like the change for style reasons.
>
> The comment about architectures with non-zero NULL is a little
> misleading, though. This change has no impact on non-zero bit pattern
> NULL architectures. The zero pointer value in C is NULL and NULL is
> the zero pointer value in C. It may have a bit pattern other than
> zero (i.e., printf("%p", NULL) may be non-zero and memset(&p, 0,
> sizeof(p)) is bogus in portable code) but assigning the logical zero
> value is always legitimate.
This is well known. Some details in the above are mis-stated:
- that this change has no effect might depend on using prototypes. The
changed areas sometimes used NULL for function args near plain 0 in
assignments. This is just a larger style bug than using either 0 or
NULL consistently, since everything in the kernel uses prototypes.
This was a bug in K&R code. Correct K&R has cast 0 or NULL in function
args, but this is painful, so nearby assignments shouldn't use the same
style. NULL can be:
(a) an integer constant expression with value 0. E.g., (2 + 2L - 4ULL)
(b) ((void *)(a))
So for unprototypes functions taking a pointer arg, uncast NULL is only
correct if NULL happens to be ((void *)(a)) and the type that the
function takes is 'void *' or possibly 'qualified void *'. (I forget
if there is magic to allow qualifiers.)
- there is no such thing as the zero pointer value. There are only
null pointer constants. Null pointer constants are (a) and (b) as
above. These arq quite different from null pointers. (a) only has
value 0 as an integer. Casting this to (void *) gives a pointer,
but I think it is still magic, so can have a different representation
than when it is converted to a null pointer:
np = (void *)0;
may either retain all-bits-0 or change them to a nonzero magic value
in the cast of 0, then convert to different magic bits (even back
to all-bits 0) in np.
- printf("%p", NULL) is not just unportable, but undefined in general,
since printf() is variadic so this is broken like the unprototyped
case. It is undefined even if pointers have all bits 0, since NULL
- the changed code retains the style bug of explicit initialization of
static null pointers. It is implicitly to null pointer constants
which get converted in the same way as integer 0 or NULL.
> After all, NULL is just a casted zero value:
>
> #define NULL ((void *)0)
Hmm, that is correct for C, but this is an implementation detail. NULL
is actually a convoluted mess: from sys/_null.h
X #ifndef NULL
X
X #if !defined(__cplusplus)
X #define NULL ((void *)0)
The non-C++ case is now simple, but this defeats the excuse for
existence of this file. This file was originally just to support C
and allow defining NULL in a central place as 0L for __LP64__ only,
while keeping the old definition as 0 for other cases, with extra
complications for __KERNEL. Only broken programs noticed the
difference. Mainly ones not using prototypes which has mostly gone
away before this change was made. The ifdef allowed exposing type
errors by changing the definition to weirder ones. 0L for __LP64__
and 0 for !__LP64__ did the reverse. ((void *)0) is even more
forgiving, except it exposes the type error of using NULL for integer 0.
Eventually the definition was changed to ((void *)0) as above. I don't
like this since it only helps broken C programs and is wrong for C++.
The file was retained to support C++, and grew messes.
X #else
X #if __cplusplus >= 201103L
X #define NULL nullptr
X #elif defined(__GNUG__) && defined(__GNUC__) && __GNUC__ >= 4
X #define NULL __null
X #else
X #if defined(__LP64__)
X #define NULL (0L)
The commit that changed the C case to use ((void *)0) was missing
parentheses. The commit that fixed this also added parentheses that
were not missing to 0L. This remains as a style bug.
X #else
X #define NULL 0
X #endif /* __LP64__ */
X #endif /* __GNUG__ */
X #endif /* !__cplusplus */
Style bugs:
- tab instead of space after #endif
- no comment on any #else, and perhaps too comments on too many #endif's.
Comments on the inner ifdefs unimprove readabilty, but the set of ifdefs
is so convoluted that it is hard to see which #else or #elseif is inner.
> Maybe this is moot. I don't believe any architecture FreeBSD actually
> supports has non-zero bitpattern NULL, but something weird like CHERI
> might.
Compilers should do portability checks on it. In fact, the convolutions
in this file do little except define nullptr and __null to let compilers
do more checking. Only the C++ case even has them. Compilers can do
most checks using just the C90 definition of null pointer constants.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170221234517.L3157>