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

[Alex Buckley]

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