Review request for JDK-8013357: Javac accepts erroneous binary comparison operations
Eric McCorkle
eric.mccorkle at oracle.com
Thu Jun 6 08:06:13 PDT 2013
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/1e566185/eric_mccorkle-0001.vcf
More information about the compiler-dev
mailing list