RFR 8208399: Metadata methods print_(value_)on_maybe_null() compare 'this' to NULL

[David Holmes]

On 31/07/2018 11:00 AM, Ioi Lam wrote:
> There are 92 cases of '[(]NULL ==' vs 3403 cases of '== NULL[)]' in the 
> hotspot source code.
> 
> It's been claimed that (NULL == variable) is superior because if you 
> type "=" by mistake, or delete one of the equal signs by mistake, the 
> compiler will catch you.

Yes well aware of the reason behind it :) I just find it jarring to read 
code expressed that way. If I'd been taught it from the beginning, or if 
hotspot had always used it ...

But unless we're planning on updating the coding guidelines to require 
this I don't see we should be making such changes "just because"

Cheers,
David

> - Ioi
> 
> 
> On 7/30/18 2:52 PM, David Holmes wrote:
>> On 31/07/2018 4:16 AM, Harold David Seigel wrote:
>>> Thanks Gerard!
>>>
>>> I'll make that change before pushing the fix.
>>
>> Hmmm is that part of our coding guidelines? It is not something we 
>> generally follow. Personally I've never liked the "if (<constant> == 
>> variable)" form as it just reads wrong to me.
>>
>> David
>>
>>> Harold
>>>
>>>
>>> On 7/30/2018 2:05 PM, Gerard Ziemski wrote:
>>>> Looks good.
>>>>
>>>> In src/hotspot/share/oops/metadata.hpp tiny nitpick - I was once 
>>>> told that instead of:
>>>>
>>>> +    if (m == NULL)
>>>>
>>>> we should be using:
>>>>
>>>> +    if (NULL == m)
>>>>
>>>> but I guess the entire file would have to use that syntax.
>>>>
>>>>
>>>>
>>>> cheers
>>>>
>>>>> On Jul 27, 2018, at 12:15 PM, Harold David Seigel 
>>>>> <harold.seigel at oracle.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Please review this JDK-12 fix for bug JDK-8208399.  The fix changes 
>>>>> function print_value_on_maybe_null() into a static function to 
>>>>> avoid comparisons between 'this' and NULL.  It also deletes unused 
>>>>> methods print_maybe_null() and print_on_maybe_null().
>>>>>
>>>>> Open Webrev: 
>>>>> http://cr.openjdk.java.net/~hseigel/bug_8208399/webrev/index.html
>>>>>
>>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8208399
>>>>>
>>>>> This fix was regression tested by running Mach5 tiers 1 and 2 tests 
>>>>> and builds on Linux-X64, Windows, Solaris Sparc, and Mac OS X, 
>>>>> running tiers 3-5 tests on Linux-x64, and by running JCK-11 Lang 
>>>>> and VM tests on Linux-x64.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>
>