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

[Eric McCorkle]

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?

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/20130603/00deb2a7/eric_mccorkle.vcf