RFR: 8240110: Improve NULL

[David Holmes]

Hi Kim,

On 20/04/2020 10:58 am, Kim Barrett wrote:
>> On Apr 19, 2020, at 6:13 PM, David Holmes <david.holmes at oracle.com> wrote:
>>
>> On 20/04/2020 3:08 am, Leo Korinth wrote:
>>> On 17/04/2020 08:45, David Holmes wrote:
>>>> 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
>>> I totally agree with you and the change needed was smaller than I rememberd, I should have fixed this directly. Returning NULL for an object that is not of pointer type seems just wrong, but returning a JVMCIObjectArray(NULL) seems okay.
>>> What about this?
>>> #define JVMCI_CHECK_0             JVMCI_CHECK_(NULL_WORD)
>>
>> Why does this integer returning macro need to return NULL_WORD? As I said above if writing this directly I would write "return 0;" in the code. Is this just an attempt under the covers to (eventually) avoid misusing JVMCI_CHECK_0 where JVMCI_CHECK_NULL should have been used?
> 
> I agree that NULL_WORD has the wrong implication here.
> 
> As you correctly surmise, the problem with returning `0` is that it is
> a null pointer constant. It would be nice if JVMCI_CHECK_0 could not
> be used in a context where a pointer result is expected, and instead
> require JVMCI_CHECK_NULL in such cases. That was the point of my
> suggestion of using
> 
> #define JVMCI_CHECK_0  JVMCI_CHECK_(int(0))
> 
> The expression `int(0)` is not a null pointer constant, so won't
> implicitly convert to a null pointer.

I'm sure I tried something like that when fixing up some misuses of the 
more general CHECK_0 macro, but it did not help in detecting the misuse.

Cheers,
David
-----

> I realized later that there's a (currently theoretical?) problem with
> this; if the result type is an unsigned integral value then this may
> (someday?) produce a signed to unsigned conversion warning. (The
> proposed use of NULL_WORD has the same problem.) I think there's not a
> way to eliminate that possibility prior to C++11, other than by
> introducing and using JVMCI_CHECK_0U to return an unsigned zero value.
> But since I think that problem is currently theoretical, and can be
> solved with C++11, I'm not concerned.
> 
>>> #define JVMCI_CHECK_NULL          JVMCI_CHECK_(NULL)
>>> and 3 changes of JVMCI_CHECK_NULL to JVMCI_CHECK_(JVMCIObjectArray(NULL))
>>
>> I'm not sure specifically where those changes are.
>>
>> In general I think I prefer to see an explicit constructor passed NULL rather than some implicit conversion, but that is a stylistic matter and the implicit conversion form seems to be used quite extensively.
> 
> The question is, given class Foo, which has a conversion from pointer
> constructor and is default constructable, which is preferable:
> 
> Foo foo1 = NULL;
> Foo foo2(NULL);
> Foo foo3;
> 
> and similarly, with functions taking a Foo:
> 
> function1(NULL)
> function2(Foo(NULL))
> function3(Foo())
> 
> We have occurrences of both variant #1 usages. The proposal is to
> instead use the variant #2 style. My own preference would be for the
> variant #3 style. Both #2 and #3 should also declare the conversion
> constructor "explicit" to actively prevent variant #1; that's missing
> from the current proposal.
> 
>