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

Sun Jan 13 12:14:01 UTC 2019

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.

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