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

[Maurizio Cimadamore]

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 --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20130530/c8bc4660/attachment.html