RFR: 8240110: Improve NULL

[Kim Barrett]

> On Mar 29, 2020, at 8:24 PM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> Hi, could I have a review on this change to improve NULL?

I've not gone through all the changes since I disagree with a core
part of this change.  No point in looking at most of the rest until
that's been discussed.

------------------------------------------------------------------------------
src/hotspot/share/utilities/null.hpp

39 // This line is put after the include guard on purpose, this will make it more likely that NULL is defined NULL (NULL can be re-defined by system headers).
40 // I have not found a good way to make the system headers not define NULL. This line really helps to remove bad definitions of NULL, though not completely.
41 #undef NULL
42 #define NULL 0

What is a "bad definition of NULL"?

I don't think we should make such a change to NULL.

As you note, it's hard to ensure this definition actually gets used.
I think trying to provide our own definition of NULL that is different
from what's provided by the standard library is just asking for
trouble, probably even more so than the assert problem.

Also, this removes some beneficial type checking on some platforms
(notably on at least some gcc platforms, and probably clang too) where
NULL is a special non-integer value (__null) that prevents arithmetic
and is guaranteed to have the same size as a pointer.

And #define'ing NULL as 0 may be actively wrong for the reasons
discussed in the #ifdef SOLARIS definition in globalDefinitions_gcc.hpp.
The referenced bug says something like that code was needed for a
broken standard library on some unknown proprietary OS; it's unclear why
that code is conditionalized on SOLARIS, since the bug seems to suggest
that Solaris header actually do the right thing.  Note also that this
code is deprecated by JEP 362.

It would be interesting to find out if the AIX problem with dirent.h
unconditionally redefining NULL still exists.

I think the not-platform-specific definition of NULL_WORD is fine, but
could just be in globalDefinitions.hpp.  But there's no ODR issue so
long as there's only one definition per translation unit, since const
has translation-unit scope.  It does mean that pointers to NULL_WORD
from different translation units compare !=, but that's expected.  And
being more careful about the use of NULL vs NULL_WORD is good.  A
non-integral definition of NULL helps find some of those.  (Though not
ones like you found in interpreterRT_aarch64.cpp.)

I don't particularly like the changes from NULL to the literal 0 null
pointer constant; I don't think that improves readability at all.  I
wouldn't object to using nullptr in those places, but we either need
to wait for C++11 or provide a C++98 implementation until then.  I
think it's possible to get fairly close to a C++98 nullptr / nullptr_t
implementation, but there are a few features I'm not sure how to
implement in C++98.  (Having a constexpr constructor being one.)

So while I think some wrong code has been turned up and should be
fixed, I don't think the attempt to change NULL should proceed.

I have no particular opinion about the goodness of NULL_WORD as a
concept.  As noted, it is widely used.

------------------------------------------------------------------------------
src/hotspot/share/runtime/os.inline.hpp
34 #include OS_HEADER_INLINE(os)
35 #undef NULL
36 #include "utilities/null.hpp"

[added lines 35-36]

If that's the sort of thing needed by the proposed change to NULL,
that's another reason to not go down that path.

------------------------------------------------------------------------------ 
src/hotspot/share/runtime/os.cpp
1324   if (path == NULL || strlen(path) == 0 || file_name_length == (size_t)NULL) {
=>
1324   if (path == NULL || strlen(path) == 0 || file_name_length == 0) {

Wow!  Nice find!

------------------------------------------------------------------------------

Regarding David's comment: "(I have to say it would nice if whenever
you saw " x = NULL" that you knew x was a pointer type.)"
Unfortunately, there's no such guarantee.  NULL is a null pointer
constant, and a null pointer constant *is* an integral constant
expression, by definition (C++98 4.10/1).