Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Thu Jun 6 13:21:32 PDT 2013
Is there any reason why isPrimitive() on the type representing Number
(or any unboxable type, for that matter) would return true? Because if
that's the case, and isSubtype handles unboxing, then it would explain
why this code works for Boxed(T) == T style comparisons.
On 06/06/13 11:41, Maurizio Cimadamore wrote:
> Ok - thx for the clarification!
>
> Maurizio
>
> On 06/06/13 16:06, Eric McCorkle wrote:
>> That's what I meant when I said it didn't work, sorry if I wasn't clear
>> about it. The code in webrev.01 does work, though. So does the
>> following code:
>>
>> public boolean isEqualityComparable(Type s, Type t, Warner warn) {
>> boolean tPrimitive = t.isPrimitive();
>> boolean sPrimitive = s.isPrimitive();
>> if (tPrimitive && sPrimitive) {
>> return isSubtype(s, t) || isSubtype(t, s);
>> } else if (!tPrimitive && !sPrimitive) {
>> return isCastable(s, t, warn) || isCastable(t, s, warn);
>> } else
>> return false;
>> }
>>
>> I haven't posted a new webrev yet, though, as I want to understand *why*
>> it works, and why the code with unboxedTypeOrType doesn't work.
>>
>> On 06/05/13 05:16, Maurizio Cimadamore wrote:
>>> All versions I've seen so far are wrong in a way or another, so I would
>>> not go back to a previous version. Have you tried the code I pasted in
>>> my last email?
>>>
>>> Maurizio
>>>
>>> On 04/06/13 23:03, Eric McCorkle wrote:
>>>> Actually that code doesn't work (it won't even get through a build).
>>>> Any kind of null == Object comparison fails. Strangely, the code in
>>>> webrev.02 seems to pass all the existing tests (including my new ones),
>>>> which seems strange to me.
>>>>
>>>> I'll backpedal to webrev.01 and simplify it. But perhaps we ought to
>>>> flag this part of the type checker for refactoring as well.
>>>>
>>>> On 06/04/13 06:08, Maurizio Cimadamore wrote:
>>>>> On 03/06/13 20:38, Eric McCorkle wrote:
>>>>>> Good suggestion, I've made the change (no new webrev, as it just
>>>>>> reorders defs).
>>>>>>
>>>>>> Are there any more suggestions from anyone, or is this one good to
>>>>>> go?
>>>>> The code is wrong - I didn't realize when writing it on the review.
>>>>> You
>>>>> only do unboxing _after_ having checked that both types are
>>>>> primitives -
>>>>> so nothing can possibly happen. I have to say I'm a bit worried
>>>>> that the
>>>>> test you wrote didn't catch this (as well as the symmetry problems
>>>>> with
>>>>> the previous implementation). I'd focus on writing a better test first
>>>>> and then tweaking the compiler code accordingly. Note that the test
>>>>> can
>>>>> be made much simpler by using the infrastructure for testing types
>>>>> available in test/tools/javac/types (which will save you the hassle of
>>>>> creating mini test cases when the only thing you are interested in is
>>>>> whether a type is 'equality comparable' to another type).
>>>>>
>>>>> The right code for the test should be this:
>>>>>
>>>>> if (allowBoxing) {
>>>>> s = unboxedTypeOrType(s);
>>>>> t = unboxedTypeOrType(t);
>>>>> }
>>>>>
>>>>> boolean tPrimitive = t.isPrimitive();
>>>>> boolean sPrimitive = s.isPrimitive();
>>>>>
>>>>> if (tPrimitive && sPrimitive) {
>>>>> return isSubtype(s, t) || isSubtype(t, s);
>>>>> } else if (!tPrimitive && !sPrimitive) {
>>>>> return isCastable(s, t, warn) || isCastable(t, s, warn);
>>>>> } else {
>>>>> return false;
>>>>> }
>>>>>
>>>>> Maurizio
>>>>>> On 06/03/13 14:22, Alex Buckley wrote:
>>>>>>> In ObjectZeroCompare, I would just suggest having the *_name and
>>>>>>> *_contents variables listed in the same order as the
>>>>>>> assert_compile_*
>>>>>>> invocations in run(). I think the most readable order is: Object,
>>>>>>> Number, IntegerSuper, Uncastable, [Flips from failure to success
>>>>>>> here]
>>>>>>> Castable, Integer, etc [as you have it already].
>>>>>>>
>>>>>>> Alex
>>>>>>>
>>>>>>> On 6/3/2013 11:11 AM, Eric McCorkle wrote:
>>>>>>>> Forgot the URL on this. It's here:
>>>>>>>> http://cr.openjdk.java.net/~emc/8013357/webrev.02/
>>>>>>>>
>>>>>>>> On 06/01/13 08:28, Eric McCorkle wrote:
>>>>>>>>> New webrev is up. I rolled back allowing Number == primitive
>>>>>>>>> comparisons (I thought I'd rolled that back in the previous),
>>>>>>>>> and I
>>>>>>>>> implemented maurizio's suggested simpler comparison. I also
>>>>>>>>> added a few
>>>>>>>>> more test cases.
>>>>>>>>>
>>>>>>>>> JPRT's been a bit unreliable, but I'm getting clean runs.
>>>>>>>>>
>>>>>>>>> Please review and comment.
>>>>>>>>>
>>>>>>>>> On 05/30/13 05:03, Maurizio Cimadamore wrote:
>>>>>>>>>> On 29/05/13 20:53, Eric McCorkle wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Please review my latest patch for this problem. Since my
>>>>>>>>>>> earlier
>>>>>>>>>>> review
>>>>>>>>>>> request, several things have happened:
>>>>>>>>>>>
>>>>>>>>>>> 1) A CCC has been submitted, reviewed (at length) and approved.
>>>>>>>>>>>
>>>>>>>>>>> 2) I've confirmed the correct behavior wrt object-primitive
>>>>>>>>>>> comparisons.
>>>>>>>>>>>
>>>>>>>>>>> 3) I've fixed some errors in our tests, which would not compile
>>>>>>>>>>> when
>>>>>>>>>>> javac enforces the correct type rules.
>>>>>>>>>>>
>>>>>>>>>>> 4) This change has been flagged for a release note.
>>>>>>>>>>>
>>>>>>>>>>> The webrev is here:
>>>>>>>>>>> http://cr.openjdk.java.net/~emc/8013357/webrev.01/
>>>>>>>>>> Hi,
>>>>>>>>>> couple of comments:
>>>>>>>>>>
>>>>>>>>>> *) I would be wary of adding Number to the symtab which will load
>>>>>>>>>> symbols eagerly; j.l.Number is not available on all platforms,
>>>>>>>>>> so there
>>>>>>>>>> will be issues when using javac in cross compilation environment,
>>>>>>>>>> compiling against a J2ME rt.jar; the usual trick is to call
>>>>>>>>>> Symbtab.
>>>>>>>>>> synthesizeEmptyInterfaceIfMissing (see for other code in Symtab
>>>>>>>>>> that
>>>>>>>>>> does that for a reference).
>>>>>>>>>>
>>>>>>>>>> *) The logic for checking equality seems questionable/overly
>>>>>>>>>> complex
>>>>>>>>>> (expanded in meta code below):
>>>>>>>>>>
>>>>>>>>>> boolean isEqualityComparable (Type s, Type t)
>>>>>>>>>> if (<both primitives>) {
>>>>>>>>>> return s <: t //1
>>>>>>>>>> } else if (<both references>) {
>>>>>>>>>> return s castable to t //2
>>>>>>>>>> } else if (source >= 1.5) {
>>>>>>>>>> if (<t is primitive>) {
>>>>>>>>>> return (<s is j.l.Number>) ? <t.isNumeric() :
>>>>>>>>>> (unbox(s) <:
>>>>>>>>>> t) //3
>>>>>>>>>> } else {
>>>>>>>>>> return (<t is j.l.Number>) ? <s.isNumeric() :
>>>>>>>>>> (unbox(t) <:
>>>>>>>>>> s) //4
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> The first thing that strikes me is that in places (1, 2, 3, 4)
>>>>>>>>>> we are
>>>>>>>>>> not being symmetric; i.e.
>>>>>>>>>>
>>>>>>>>>> isEqualityComparable(int, long) = true
>>>>>>>>>> isEqualityComparable(long, int) = false
>>>>>>>>>>
>>>>>>>>>> The second thing that worries me is that the spec says something
>>>>>>>>>> like:
>>>>>>>>>>
>>>>>>>>>> *) if both are numeric (i.e. primitives) and one is
>>>>>>>>>> convertible to
>>>>>>>>>> numeric (i.e. unboxable), then a numeric comparison is performed
>>>>>>>>>>
>>>>>>>>>> I think this means that the code should do some check to see
>>>>>>>>>> whether
>>>>>>>>>> numeric comparison is applicable, (i.e. eventually unboxing) and
>>>>>>>>>> then
>>>>>>>>>> calling appropriate routine to do the comparison test.
>>>>>>>>>>
>>>>>>>>>> Also, restating a point raised by Alex, Number == primitive is
>>>>>>>>>> not
>>>>>>>>>> allowed as Number is not convertible to a numeric type; I
>>>>>>>>>> think the
>>>>>>>>>> test
>>>>>>>>>> should be something like:
>>>>>>>>>>
>>>>>>>>>> boolean isEqualityComparable(Type s, Type t)
>>>>>>>>>> if (<both primitives>) {
>>>>>>>>>> if (source >= 1.5) {
>>>>>>>>>> s = types.unboxedTypeOrType(s);
>>>>>>>>>> t = types.unboxedTypeOrType(t);
>>>>>>>>>> }
>>>>>>>>>> return s <: t || t <: s;
>>>>>>>>>> } else if (<both references>) {
>>>>>>>>>> return (s castable to t) || (t castable to s);
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Maurizio
>>>>>>>>>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: eric_mccorkle.vcf
Type: text/x-vcard
Size: 314 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130606/7308f1d6/eric_mccorkle.vcf
More information about the compiler-dev
mailing list