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

Sun Jan 13 19:28:38 UTC 2019

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".

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
>>
>>