RFR: 8240110: Improve NULL

Tue Mar 31 21:11:20 UTC 2020

On 31/03/2020 14:06, Kim Barrett wrote:
>> 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"?

One that we have not defined in null.hpp

> 
> 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.

I would love to change all occurrences of NULL to nullptr, that would 
solve /all/ problems but the diff would be huge. I do not think 
reviewers will allow such a massive change although that would be my 
first choice. Thus the idea to define NULL to nullptr. At least after 
this change we only have one alternative definition of NULL.

> 
> 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.

Type checking will be vastly improved with c++11. You can see all kind 
of "errors" I found in the code already. I do agree with __null though 
and I will #ifdef it in null.hpp (as I said in the reply to David, sorry 
for not getting it the original webrev). But nullptr clearly is superior 
to __null.
> 
> 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.

The change "seems" to work, and "should" work.

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

GNU headers also redefine NULL unconditionally, and as I said in the 
mail I do have a patch that #undef NULL after all system headers. 
However this is no problem /I/ have introduced or made worse (to my 
knowledge).
> 
> 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.)

Have you read null.hpp in part1 of the patch? I feel you miss a lot of 
context. I do /not/ want to define NULL to 0, I want to do it 
temporarily. I do not want to throw away my fixes, but instead prepare 
for your move to later version of c++.

> 
> 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.

Two questions:

1) Is my change in any way worse than what we have today (after I fix 
__null)?

2) Would it not be nice for you to just change the NULL #define to 
nullptr when you fix more modern c++?

> 
> 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.

I would like to have a better solution to system headers that 
unconditionally redefines NULL, unfortunately I have found none. Please 
help me find the better 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!

__null can be casted to integral, my failed NULL could not.
> ------------------------------------------------------------------------------
> 
> 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).
> 

We can define NULL better than the standard (as nullptr).

Lastly, I do think moving NULL to null.hpp is highly beneficial to the 
current code base. If that can not be accepted by you I will try to at 
least push as many of the "fixes" as possible. But I still hope that you 
will like this and --- maybe --- I just failed to communicate the idea 
in a good way.

One alternative is to remove our NULL definitions altogether and replace 
NULL with nullptr. I would prefer that. Would you? Do you actually think 
we could get such a patch reviewed?

nullptr is so important for type safety and template specialization, I 
feel every step in that direction is important.

Thanks,
Leo