[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8212959: Remove booleans from tests in vmTestbase
From: David Holmes <david.holmes () oracle ! com>
Date: 2019-01-13 21:06:01
Message-ID: 60fd5819-5c47-1dfe-5481-3e152b987ce6 () oracle ! com
[Download RAW message or body]
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@oracle.com <mailto:david.holmes@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@oracle.com <mailto:david.holmes@oracle.com>
>>>> > <mailto:david.holmes@oracle.com
>>>> <mailto:david.holmes@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
>>>
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic