Andrew Haley wrote:
Stuart Ballard writes:
> Andrew Haley <aph <at> redhat.com> writes:
> > No, that's not what I mean by illegal. It's illegal in the sense that
> > the specification places requirements on the implementors of
> > subclasses, and that a subclass which does not meet these requirements
> > is not a well-defined Java program.
>
> public class Foo {
> private String x;
> public Foo(String x) {
> this.x = x;
> }
> public boolean equals(Object o) {
> if (!(o instanceof Foo)) return false;
> return x.equals(((Foo) o).x);
> }
> public int hashCode() {
> return x.hashCode();
> }
> }
>
> This class is clearly intended to meet the spec of equals() and hashCode() and
> does so correctly in the vast majority of cases. But not quite: new
> Foo("").equals(new Foo(null)) is false, but new Foo(null).equals(new
Foo("")) is
> NPE.
>
> Is this not a well-defined Java class?
AFAICS it isn't: the spec requires equals() to be symmetric for all
non-null x and y:
"for any non-null reference values x and y, x.equals(y) should return
true if and only if y.equals(x) returns true."
The result of any operation that involves this class is therefore not
well-defined, and whatever happens, it's not a bug.
Allow me to split the discussion in two parts:
1. commit the patch
2. add a mauve testlet
1:
Are you against or for committing this patch? Everyone so far agreed to
commit it. I think the reason is clear: Keep classpath as usable as
possible and don't handicap users. They might not even be responsible
for the bug in the code they use.
2:
A mauve testlet is not really required. A comment would do the same
thing as well (see new attached patch).
Anyway, a mauve testlet would be more reliable. There's no need for a
not legal or not well-defined program. See attached example.
Marco
Index: classpath/java/util/Arrays.java
===================================================================
--- classpath/java/util/Arrays.java (Revision 119819)
+++ classpath/java/util/Arrays.java (Arbeitskopie)
@@ -370,10 +370,12 @@
while (low <= hi)
{
mid = (low + hi) >>> 1;
- final int d = Collections.compare(key, a[mid], c);
+ // NOTE: Please keep the order of a[mid] and key. Altought not required
+ // by the specs, a Sun JRE has it in that order as well.
+ final int d = Collections.compare(a[mid], key, c);
if (d == 0)
return mid;
- else if (d < 0)
+ else if (d > 0)
hi = mid - 1;
else
// This gets the insertion point right on the last loop
import java.util.Arrays;
import java.util.Comparator;
public class ArraysTest
{
public static void main(String[] args)
{
Comparator comp = new Comparator()
{
public int compare(Object o1, Object o2)
{
if(o1 instanceof String)
{
int i1 = Integer.parseInt((String)o1);
int i2 = ((Integer)o2).intValue();
return (i1 == i2 ? 0 : (i1 < i2 ? -1 :
1));
} else
{
System.out.println("Expected the
objects in different order!");
System.out.println("This is not
required by the spec, but that's how a Sun JRE does it.");
int i1 = ((Integer)o1).intValue();
int i2 = Integer.parseInt((String)o2);
return (i1 == i2 ? 0 : (i1 < i2 ? -1 :
1));
}
}
};
Object[] o1 = { "22", "23", "24" };
Object o2 = new Integer(23);
System.out.println(Arrays.binarySearch(o1, o2, comp));
}
}
