Andrew Haley wrote:
Marco Trudel writes:
> Casey Marshall wrote:
> > On Dec 13, 2006, at 1:28 PM, Marco Trudel wrote:
> >
> >> David Daney wrote:
> >>> Casey Marshall wrote:
> >>>> In general I really urge against going out of our way to support
> >>>> behavior like this.
> >>
> >> Can you explain that further? You're against changes that
> >> - keep the specification as correct as they were before
> >> - will adapt classpath to the RI behavior
> >> - will prevent users from running into problems
> >> - restore old classpath behavior (regression fix)
> >> - have no negative side effects whatsoever
> >>
> >> Sorry if that seems to be a mean summary. This are just the
> >> points I'm seeing and I'm actually really surprised about the
> >> number and kind of reactions to that patch or that the mentioned
> >> bugreport has been closed as invalid.
> >
> > It is not a bug. 'Nuff said. I was replying to David, not to you.
>
> I agree, it's not a bug in classpath. The bug is in the comperator. I
> don't say anything else...
Right, so if it isn't a bug it can't be a regression.
True! But I made a bug in my patch, fixed in the attached one.
> >> So as I said, I don't mean to offend you, I just can't follow
> >> why we wouldn't follow the RI when we still fulfill the
> >> specifications.
> >>
> >
> > I did say that I was against *going out of our way* to adhere to
> > the precise behavior of the RI, when neither the spec nor the
> > algorithms used require it.
> >
> > It's fine if you want to go looking for things like this, and
> > writing fixes for them. These are not a bugs, though, and are not
> > a priority.
>
> Absolutely. There's no need to actively search such problems. But
> if I stumbles over one, I will adapt classpath to follow the RI and
> thus make the life of classpath users a tiny little bit easier...
The problem with this kind of approach is that it is, in practice,
impossible to maintain. Anywhere that uses a comparator might well
break this again. In practice, that's almost certain to happen.
So, we can change this now, but it will break in the future. Maybe
tomorrow, maybe next week, but it will break.
Do you mean if we change this now to be identical to the Sun behavior,
it will break bad comparators written for the classpath version of
Arrays.binarySearch()?
Well yes, good point. I didn't think about that...
This seems to go deeper than I initially thought. So it's a decision of
either be compliant with a Sun JRE and thus allow bad code developed
against a Sun JRE to work or keep it as it is and allow bad code
developed against classpath to work.
IMHO, it should be changed because the codebase developed against a Sun
JRE is significantly bigger. What do others think?
It's interesting to compare gcc's approach to this problem. In
general we refuse to change the compiler when it is standards-
conforming, but in a few cases where a user error is so widespread
that fixing it would be onerous, we add a flag to gcc to be compatible
with the old broken code. This is the case with some kernel code,
which had to be compiled with -fno-strict-aliasing.
Unfortunately it's not just onerous to fix that bug (wrong implemented
comparator) if it's in a closed source library, it's just not possible.
I'm currently in that position. I didn't wrote that comparator in
question myself, it's in a closed source library. So I now can either:
1. decompile the library and fix it (legally questionable)
2. start my own fork of classpath with my patch
3. hope that the classpath people will accept my patch
I'm in the fortunate situation where I can choose between 2 and 3. But
there are many people out there who can't.
> >>> I changed my mind! As a matter of principle, since I had to
> >>> fix my code, everyone should have to suffer in the same manner,
> >>> as their penalty for disobeying the mandates of the
> >>> specification! :)
> >>
> >> Whaaat? David! I really hope - for the sake of humanity - that
> >> you're joking. Ah right, there's a ":)" at the end. I was so
> >> shocked from that statement that I missed it first...
> >>
> >
> > In this case, the Comparator is still wrong. It may work now with
> > Classpath's binarySearch, but it is *in general* broken. If it
> > gets used with some other algorithm, it may not work.
> >
> > HIDING BUGS IN CODE IS BAD.
>
> What are we actually discussing about? I think we have the same
> opinion?!
Part of the problem here is the use of the phrase "reference
implementation". Over the years, language implementors have come to
realize that having a reference implementation as a basis is a bad
idea. Instead, more or less formal specifications have come to be
used, and this has the enormous benefit of formalizing what is known
to be unchanging and what is not. This means that maintenance
programmers know what externally-visible behaviour may be altered, and
what may not.
If you have to be compatible with a reference implementation, you
can't change *anything* that might make any difference to an
application, no matter how tiny. The idea that one must not only meet
the spec but also not diverge from the RI, therefore, imposes a
straightjacket on Classpath developers; that is positively harmful.
I don't fully understand what you say in this last paragraph. But from
what I understand, I have to say that have the same opinion. Follow the
specs, not the specs and the RI. But if problems are found where the RI
can be met while keeping the specs, it should be done.
Marco
Index: classpath/java/util/Arrays.java
===================================================================
--- classpath/java/util/Arrays.java (Revision 119819)
+++ classpath/java/util/Arrays.java (Arbeitskopie)
@@ -370,10 +370,10 @@
while (low <= hi)
{
mid = (low + hi) >>> 1;
- final int d = Collections.compare(key, a[mid], c);
+ 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
