RFR (M) 8212959: Remove booleans from tests in vmTestbase

David Holmes david.holmes at oracle.com
Sun Jan 13 21:06:01 UTC 2019 

On 14/01/2019 5:28 am, Chris Plummer wrote:
> On 1/13/19 4:14 AM, David Holmes wrote:
>> On 13/01/2019 8:33 am, Chris Plummer wrote:
>>> Hi JC and David,
>>>
>>> Sorry I'm late here. Was out most of the week. I assume by "implicit 
>>> boolean" that David means treating an int as a boolean when the the 
>>> int could potentially contain a value other than 0 or 1? So if we have:
>>
>> The style guide [1] states:
>>
>> "Do not use ints or pointers as booleans with &&, ||, if, while. 
>> Instead, compare explicitly != 0 or != NULL, etc. (See #8 above.)"
>>
>>>
>>>        int success;
>>>      ...
>>>        if (success != NSK_TRUE) {
>>>
>>> You feel the check against NSK_TRUE is needed, and using !success 
>>> would potentially yield an incorrect evaluation of the "boolean".
>>
>> No it isn't a correctness issue (though obviously you can use an int 
>> incorrectly in such a context) it's a style issue. We regularly 
>> eradicate implicit booleans so it is inappropriate to introduce them 
>> here.
> 
> Ok, but the above code is still incorrect. At least !success would have 
> always have given you the right result, so in this case the rule to add 
> an explicit comparison has lead to a bug (although in reality it a bug 
> that is never exposed). It should be "success == NSK_FALSE".

That's a weird attempted justification. If "success" is not initialized 
or can take on any value other than NSK_TRUE or NSK_FALSE then that is a 
logic bug pure and simple and its irrelevant how success is tested in a 
condition.

Not that these tests would have been written to the hotspot style guide 
in the first place. This all goes back to these basically being C-style 
tests with no C++ bool used.

David

> 
> Chris
> 
>>
>> David
>> -----
>>
>> [1] https://wiki.openjdk.java.net/display/HotSpot/StyleGuide
>>
>>  Well, if
>>> success is not 0 or 1, then let's say it is 2. You end up with 2 != 
>>> NSK_TRUE, which evaluations true. Compared with !2 which evaluations 
>>> false. So clearly there is a difference, but which would you consider 
>>> correct in this case? I'd consider the false evaluation correct. If 
>>> success is not 0, we want !success to be false. But if you compare 
>>> against NSK_TRUE, the evaluation only ends up being false when 
>>> success == 1. This doesn't seem right to me, so I would argue that 
>>> "success != NSK_TRUE" is not only clumsy coding, but also gives the 
>>> wrong result for values other than 0 or 1. In order to correct it, 
>>> you would use "success == NSK_FALSE". But I don't understand the 
>>> implicit boolean concern with !success when the value is suppose to 
>>> always be 0 or 1. Where I would object to do something like this is 
>>> with pointer types. Using !ptr is bad. "ptr == NULL" should be used.
>>>
>>> That being said, if JC prefers to clean this up at a later date as 
>>> part of changing success to a bool, that's fine by mean also.
>>>
>>> thanks,
>>>
>>> Chris
>>>
>>>
>>> On 1/10/19 9:20 PM, JC Beyler wrote:
>>>> Hi David,
>>>>
>>>> Fair enough. I looked a bit to doing it anyway  and it is 
>>>> self-contained in a lot of cases but a lot of times the work would 
>>>> be greatly simplified if we first factorized utility methods across 
>>>> test suites first and then (or at the same time) moved them to using 
>>>> bools and not doing the explicit tests.
>>>>
>>>> So, except if  anyone  objects, I'm closing this bug as won't fix 
>>>> and I created https://bugs.openjdk.java.net/browse/JDK-8216533 and 
>>>> https://bugs.openjdk.java.net/browse/JDK-8216534 for the 
>>>> events/hotswap tests which were the most cases of this anyway.
>>>>
>>>> Thanks for the hold up David :-),
>>>> Jc
>>>>
>>>> On Tue, Jan 8, 2019 at 11:14 PM David Holmes 
>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>>     On 9/01/2019 4:43 pm, JC Beyler wrote:
>>>>     > Hi David,
>>>>     >
>>>>     > I was not planning on doing it but we could. This change came
>>>>     from a
>>>>     > request to remove the booleans from tests for a prior webrev
>>>>     change.
>>>>     > I've seen previous conversations about wanting to test explicitly
>>>>     > against JNI_TRUE but thought this was a slightly different case.
>>>>
>>>>     The basic hotspot style rule is "avoid implicit booleans".
>>>>
>>>>     > Basically, from your comment, it seems I could go three ways for
>>>>     this
>>>>     > change:
>>>>     >    1) Not do it and close the bug as won't fix :-)
>>>>
>>>>     Certainly the path of least resistance. :)
>>>>
>>>>     >    2) Go the extra step and change the various variables being
>>>>     tested to
>>>>     > bool
>>>>     >
>>>>     > My preference is to do (2) as much as possible; any case that
>>>>     cannot be
>>>>     > put in a boolean form without having major changes to the code
>>>>     base, I'd
>>>>     > leave as is. And then call it a day for this subject.
>>>>     >
>>>>     > What do you think (ie in cases where the variable can be set to
>>>>     a bool,
>>>>     > do it and then have the test be an implicit test)?
>>>>
>>>>     It's really up to you. It seems a lot of work and I don't know 
>>>> if you
>>>>     can actually push this all the way through without needing some
>>>>     int<->bool conversion somewhere anyway.
>>>>
>>>>     Cheers,
>>>>     David
>>>>
>>>>     > Thanks!
>>>>     > Jc
>>>>     >
>>>>     > On Tue, Jan 8, 2019 at 10:31 PM David Holmes
>>>>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>>>     > <mailto:david.holmes at oracle.com
>>>>     <mailto:david.holmes at oracle.com>>> wrote:
>>>>     >
>>>>     >     Hi Jc,
>>>>     >
>>>>     >     On 9/01/2019 4:12 pm, JC Beyler wrote:
>>>>     >      > Hi all,
>>>>     >      >
>>>>     >      > Fixing up the tests in vmTestbase to not be testing
>>>>     explicitly
>>>>     >     against
>>>>     >      > NSK_TRUE/NSK_FALSE. Here is the webrev to do that:
>>>>     >      >
>>>>     >      > Bug: https://bugs.openjdk.java.net/browse/JDK-8212959
>>>>     >      > Webrev:
>>>>     http://cr.openjdk.java.net/~jcbeyler/8212959/webrev.00/
>>>>     >
>>>>     >     Hold up! We don't do explicit tests when the variable is a
>>>>     bool/boolean
>>>>     >     but when it is an int, as here, we do.
>>>>     >
>>>>     >     Are you planning on converting everything to use bool?
>>>>     >
>>>>     >     Cheers,
>>>>     >     David
>>>>     >
>>>>     >      > Thanks,
>>>>     >      > Jc
>>>>     >
>>>>     >
>>>>     >
>>>>     > --
>>>>     >
>>>>     > Thanks,
>>>>     > Jc
>>>>
>>>>
>>>>
>>>> -- 
>>>>
>>>> Thanks,
>>>> Jc
>>>
>>>
> 
>

More information about the serviceability-dev mailing list