Review request for JDK-8013357: Javac accepts erroneous binary comparison operations

[Eric McCorkle]

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/20130603/908babdb/eric_mccorkle.vcf