Thanks, Alan.  You're a good reviewer.<br><br>As you suggested, I added the Classpath exception wording <br>to TimSort.java and ComparableTimSort.java.<br><br>I excised the old mergesort code from Arrays.java.<br><br>webrev regenerated.<br>
<br>Adding all-or-nothing wording would be good to add,<br>perhaps to the class-level javadoc.  But it doesn't<br>have to be part of this change.<br><br>The JDK project has unusually high compatibility concerns.<br>I and Josh believe the introduction of a possible IAE if the<br>
comparator doesn't satisfy its contract is the right thing,<br>but we'd also be willing to change the code to swallow the IAE<br>or to do so conditionally based on the value of a system property.<br>Keep in mind that a call to foreign code can throw any throwable at all,<br>
so a rogue comparator can cause arbitrary behavior even today.<br>It's up to Sun/CCC.<br><br>(There is the deeper governance issue of who gets to make<br>such decisions.  I would like most of such decisions to be made<br>
by the "group" of engineers who do the work.  For collections/concurrency,<br>such a group has worked informally for many years.)<br><br>Martin<br><br><div class="gmail_quote">On Tue, Jun 30, 2009 at 03:04, Alan Bateman <span dir="ltr"><<a href="mailto:Alan.Bateman@sun.com">Alan.Bateman@sun.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Martin Buchholz wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div class="im">
Hi sort team!<br>
<br>
Google would like to contribute a new implementation for sorting of Object arrays,<br>
which has much better performance for input that is already partially sorted,<br>
based on Tim Peter's sort used in Python.<br>
<br>
This sort is already being used in the java.util. that comes with Android.<br>
<br>
Written by Josh Bloch.<br>
<br>
</div><a href="http://cr.openjdk.java.net/%7Emartin/timsort/" target="_blank">http://cr.openjdk.java.net/~martin/timsort/</a> <<a href="http://cr.openjdk.java.net/%7Emartin/timsort/" target="_blank">http://cr.openjdk.java.net/%7Emartin/timsort/</a>><div class="im">
<br>
<br>
Strictly speaking, no further review may be necessary,<br>
since it has already seen much review by Google engineers,<br>
(including some who are OpenJDK committers),<br>
and it has seen real-world usage.<br>
<br>
Nevertheless, interested parties are invited to further review it.<br>
<br>
The proposed webrev includes some very minor change to<br>
the javadoc for Arrays.sort, that we would like to include,<br>
but are also content leaving out, or to have a Sun engineer<br>
shepherd through CCC (perhaps Chris or Alan?).<br>
</div></blockquote>
The results look very good.<br>
<br>
I assume the only contentious issue is going to be the IAE when a broken implementation of Comparable is detected. When you say "also content leaving out", do you mean just the spec update, or do you mean that the exception won't be thrown if detected? I wonder if this will need a compatibility switch so as not to change the failure mode of existing (broken) applications.  Also, I assume failure atomicity is not possible here (right?); just wondering if this needs to be stated in the javadoc for this exception.<br>

<br>
I haven't studied the code but from a brief glance, I think the "Classpath" exception might be missing from the new files in src/share/classes/java/util. Also, should the mergeSort implementation be removed or moved out of Arrays?<br>
<font color="#888888">
<br>
-Alan.<br>
<br>
</font></blockquote></div><br>