[musl] Re: [PATCH] Define NULL as nullptr when used in C++

[musl] Re: [PATCH] Define NULL as nullptr when used in C++

[Colin Cross]

On Sun, Aug 15, 2021 at 05:51:57PM +0200, Ismael Luceno wrote:
> This should be safer for casting and more compatible with existing code
> bases that wrongly assume it must be defined as a pointer.

This seems to meet the C++ spec for NULL [1], but I noticed some
compatibility issues with code that was previously compiling with
glibc.

I've found multiple places that used reinterpret_cast<int*>(NULL),
which now fail with:
error: reinterpret_cast from 'nullptr_t' to 'int *' is not allowed
According to [2] those should technically be static_cast and not
reinterpret_cast.

I've also seen failures with returning NULL from a function with a
jlong return type:
error: cannot initialize return object of type 'jlong' (aka 'long')
with an rvalue of type 'nullptr_t'

glibc uses __null if __GNUG__ is set, ((void*)0) for __cplusplus, or 0 for C.

This also meets the C++ spec for NULL [1], but is an improvement over
the previous 0L because it can be correctly interpreted as a NULL
sentinel value by Clang's -Wsentinel warning.

Ismael, can you give an example of the code that assumes NULL is a
pointer?  Does it work with __null (assuming you're using a compiler
that has GNU extensions like __null)?

In any case, I'll fix the technically incorrect code I have access to
so that it works with nullptr.

[1] https://en.cppreference.com/w/cpp/types/NULL
[2] https://en.cppreference.com/w/cpp/language/reinterpret_cast

Re: [musl] Re: [PATCH] Define NULL as nullptr when used in C++

[Rich Felker]

On Thu, Dec 23, 2021 at 11:13:01AM -0800, Colin Cross wrote:
> On Sun, Aug 15, 2021 at 05:51:57PM +0200, Ismael Luceno wrote:
> > This should be safer for casting and more compatible with existing code
> > bases that wrongly assume it must be defined as a pointer.
> 
> This seems to meet the C++ spec for NULL [1], but I noticed some
> compatibility issues with code that was previously compiling with
> glibc.
> 
> I've found multiple places that used reinterpret_cast<int*>(NULL),
> which now fail with:
> error: reinterpret_cast from 'nullptr_t' to 'int *' is not allowed
> According to [2] those should technically be static_cast and not
> reinterpret_cast.

This is an improvement then. reinterpret_cast is very very wrong here
and should produce a compile error. It's the equivalent of writing (in
C):

	*(int **)&(void *){NULL}

instead of

	(int *)NULL

i.e. it's type punning where the author intended a value conversion.

> I've also seen failures with returning NULL from a function with a
> jlong return type:
> error: cannot initialize return object of type 'jlong' (aka 'long')
> with an rvalue of type 'nullptr_t'

This also seems to be working as intended, catching a type error
(although the code was valid in earlier C++ versions).

> glibc uses __null if __GNUG__ is set, ((void*)0) for __cplusplus, or 0 for C.

I think you have cases 2 and 3 reversed. We've intentionally never
done case 1 because it's arguably non-conforming (especially in older
C++ versions) where stringifying the expansion of NULL could observe
that it's not actually an integer constant expression.

The later (C++11) allowance for it to be nullptr offered a way to do
this that is conforming and that catches more programming errors,
which was why folks were able to convince me to finally adopt it even
though I repeatedly rejected the __null thing.

> This also meets the C++ spec for NULL [1], but is an improvement over
> the previous 0L because it can be correctly interpreted as a NULL
> sentinel value by Clang's -Wsentinel warning.
> 
> Ismael, can you give an example of the code that assumes NULL is a
> pointer?  Does it work with __null (assuming you're using a compiler
> that has GNU extensions like __null)?

I don't think the main aim here is to support code that wrongly
assumes NULL has pointer type, but to catch wrong code that's assuming
it doesn't or just accidentally doing something even more wrong.
Avoiding the warning spam compiling GCC itself is kinda nice but it's
a bonus.

> In any case, I'll fix the technically incorrect code I have access to
> so that it works with nullptr.

Sounds like a good plan.

Rich

Re: [musl] Re: [PATCH] Define NULL as nullptr when used in C++

[Ismael Luceno]

On 23/Dec/2021 16:05, Rich Felker wrote:
> On Thu, Dec 23, 2021 at 11:13:01AM -0800, Colin Cross wrote:
<...> 
> > This also meets the C++ spec for NULL [1], but is an improvement over
> > the previous 0L because it can be correctly interpreted as a NULL
> > sentinel value by Clang's -Wsentinel warning.
> > 
> > Ismael, can you give an example of the code that assumes NULL is a
> > pointer?  Does it work with __null (assuming you're using a compiler
> > that has GNU extensions like __null)?

I think there was a discussion about __null before, wasn't it?

Even if nobody had an opinion on it, I would still try to avoid an
extension if there's alternatives...

> I don't think the main aim here is to support code that wrongly
> assumes NULL has pointer type, but to catch wrong code that's assuming
> it doesn't or just accidentally doing something even more wrong.
> Avoiding the warning spam compiling GCC itself is kinda nice but it's
> a bonus.
> 
> > In any case, I'll fix the technically incorrect code I have access to
> > so that it works with nullptr.
> 
> Sounds like a good plan.

+1

Re: [musl] Re: [PATCH] Define NULL as nullptr when used in C++

[Rich Felker]

On Thu, Dec 23, 2021 at 04:05:22PM -0500, Rich Felker wrote:
> On Thu, Dec 23, 2021 at 11:13:01AM -0800, Colin Cross wrote:
> > On Sun, Aug 15, 2021 at 05:51:57PM +0200, Ismael Luceno wrote:
> > > This should be safer for casting and more compatible with existing code
> > > bases that wrongly assume it must be defined as a pointer.
> > 
> > This seems to meet the C++ spec for NULL [1], but I noticed some
> > compatibility issues with code that was previously compiling with
> > glibc.
> > 
> > I've found multiple places that used reinterpret_cast<int*>(NULL),
> > which now fail with:
> > error: reinterpret_cast from 'nullptr_t' to 'int *' is not allowed
> > According to [2] those should technically be static_cast and not
> > reinterpret_cast.
> 
> This is an improvement then. reinterpret_cast is very very wrong here
> and should produce a compile error. It's the equivalent of writing (in
> C):
> 
> 	*(int **)&(void *){NULL}
> 
> instead of
> 
> 	(int *)NULL
> 
> i.e. it's type punning where the author intended a value conversion.

I've been informed I'm probably wrong about this, because
reinterpret_cast means something different when applied to pointers
than to other types. Still, getting the error is right as far as I can
tell, since reinterpret_cast is not supposed to accept nullptr_t for
conversion to int*.

Rich