RFR: 8240110: Improve NULL

[Magnus Ihse Bursie]

On 2020-04-14 15:53, Leo Korinth wrote:
> Hi!
>
> On 06/04/2020 01:37, David Holmes wrote:
>
>>>
>>> I agree that it could be compared against NULL_WORD, do you want me 
>>> to change it? (I have no opinion on this issue)
>>
>> If we're trying to clean up the use of 0, NULL, NULL_WORD then I 
>> think it should be changed.
>
> Fixed in several locations.
>
>>>>
>>>>  > new NULL value does unfortunately not convert to method pointers
>>>>
>>>> Is that a temporary limitation? I'm not sure what progress were are 
>>>> making if some pointers can use NULL and others must use 0.
>>>
>>> I believe this to be a temporary limitation, my understanding is 
>>> that when we get c++11 nullptr will convert to method pointers as 
>>> well, I could and think I should revert these changes as I did 
>>> revert my implementation of NULL, would you like that? My null_t 
>>> type could, to my knowledge, not auto convert to method pointers at 
>>> least not without template varargs stuff that comes in later 
>>> versions of the standard.
>>
>> This would look better to me as NULL.
>
> I have reverted this and a few other places.
>
>
>>>
>>> This is the same issue as before with string literals and macros, my 
>>> arguing is that I will not try to prove that value is (or in the 
>>> future always will be) a string literal, instead I will make the 
>>> code work for both string literals and pointers. I hope you do not 
>>> mind the extra parentheses I added around "value".
>>
>> Normal style rules for macros say to use the macro arg in parentheses 
>> - but there are three other uses in the same macro.
>>
>
> I will keep the parenthesis on the line I changed and do no additional 
> such change.
>
>
>>
>>> I could change CHECK_0 to use NULL_WORD though and if I do, I get 
>>> compiler errors. CHECK_0 is used for both pointers and integral 
>>> zeroes and types that implicitly converts from either. What the 
>>> original thought was is unclear to me; is CHECK_0 to be used to 
>>> return things convertible from pointers? If so, what is CHECK_NULL for?
>>
>> No CHECK_0 is intended for int-returning functions (which would also 
>> extend to intptr_t).
>
> It might be intended for it, but it is not used in such a way 
> (therefore my remark on compile failures when using NULL_WORD)
>
> I will do no change here.
>
>
>>>
>>>> ---
>>>>
>>>> src/hotspot/share/utilities/globalDefinitions_solstudio.hpp
>>>>
>>>> It isn't clear that the issue of passing 0 as NULL in a varargs 
>>>> situation is now fixed in Sol Studio.
>>>>
>>>
>>> I guess tier1-3 would fail if not? Maybe that is a false assumption 
>>> of mine?
>>
>> I would consider that a necessary but not sufficient condition.
>>
>
> I reverted my NULL/NULL_WORD redefinition so it is no issue any more 
> at least.
>
>>>>
>>>> src/hotspot/share/utilities/linkedlist.hpp
>>>>
>>>> -    return equal<E>(_data, t, NULL);
>>>> +    return equal<E>(_data, t, 0);
>>>>
>>>> Again concerned about the method/function pointer situation.
>>>
>>> I am not concerned :-) but I will revert this as it is not necessary 
>>> now or in the future as I reverted my original implementation and 
>>> c++11 will fix this.
>>
>> Ok.
>
> Fixed
>
> In additions to these changes I have also /reverted/ all changes to 
> NULL/NULL_WORD definitions. In the end Kim did not like them, and they 
> can be fixed later when we get a more modern C++. To reflect this I 
> will also change the CR title to "Improve NULL usage"
>
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/8240110/part1 (NULL)
> http://cr.openjdk.java.net/~lkorinth/8240110/part2 (NULL_WORD)
> http://cr.openjdk.java.net/~lkorinth/8240110/part3 (reverse somewhat)
>
> new in this mail:
> http://cr.openjdk.java.net/~lkorinth/8240110/part4 (improvements 
> suggested by David)
> http://cr.openjdk.java.net/~lkorinth/8240110/part5 (reverse my changes 
> to NULL/NULL_WORD itself)
>
> total combined change:
> http://cr.openjdk.java.net/~lkorinth/8240110/1

Combined change ended up containing updated copyright in 
src/hotspot/share/runtime/sweeper.hpp, even though no actual change were 
done there in the end.

/Magnus
>
> Testing:
> running tier 1-3 (looks good so far)
>
> Thanks,
> Leo