RFR: 8240110: Improve NULL

[David Holmes]

Hi Leo,

I've taken a look at the complete set of changes and the only thing I 
strongly object to is the change to:

src/hotspot/share/jvmci/jvmciExceptions.hpp

as it just doesn't make any sense to me. If you were to write the 
expanded macros out in full you would write the code as it currently is 
defined in the macro. If a method returns a pointer you would write:

return NULL;

If a method returns a jint/jbyte/jlong etc you would write

return 0;

Cheers,
David

On 14/04/2020 11:53 pm, 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
> 
> Testing:
> running tier 1-3 (looks good so far)
> 
> Thanks,
> Leo