Marco Trudel writes:
> Andrew Haley wrote:
> > Marco Trudel writes:
> > > Andrew Haley wrote:
> > > > Marco Trudel writes:
> > > > > Hey guys
> > > > >
> > > > > java.util.Arrays.binarySearch(Object[] a, Object key, Comparator
> > c)
> > > > > exchanges a[i] and key, this can lead to ClassCastExceptions as
> > shown in
> > > > > ComparatorTest.java. Arrays.patch fixes it.
> > > > >
> > > > >
> > > > > Changelog suggestion:
> > > > >
> > > > > 2006-12-13 Marco Trudel <[EMAIL PROTECTED]>
> > > > >
> > > > > * classpath/java/util/Arrays.java (binarySearch(Object[] a,
> > Object key,
> > > > > Comparator c)):
> > > > > Fix swapped objects inCollections.compare(...) call.
> > > > >
> > > > >
> > > > > Comments?
> > > >
> > > > I guess I don't get it. Arrays.binarySearch() is defined to be
> > > > searching for a key in an array. The comparator has to be able to
> > > > compare keys and values. How is this not a bug in the test case?
> > >
> > > The key might be a different object that the values. The comparator
> > > will then be responsible to see if they're equal. Thus the order is
> > > important...
> >
> > OK, but where does it say that in the spec?
>
> Sorry, I did not read the spec.
What!!
> This somehow seems logical to me.
But why? Are you assuming that the key _must_ occur on the LHS of the
Comparator?
> Also this is a regression in classpath that was introduced with a
> refactoring of binarySearch(...) and a Sun JVM does it the logical way
> too. Even if it's considered unlogical, the swap wouldn't affect it
> then... So I guess this is safe to commit.
It's perfectly safe.
You asked for comments. My comment is that your Comparator is broken,
and that a perfectly correct comparator without the bug may be
written:
public int compare(Object o1, Object o2)
{
int i1 = Integer.parseInt(o1.toString());
int i2 = Integer.parseInt(o2.toString());
return (i1 == i2 ? 0 : (i1 < i2 ? -1 : 1));
}
Andrew.
