Joe, AbstractStringBuilder is a mutable class. You might need to take a local copy and do the explicit boundary check before going into the StringLatin1/UTF16, especially StringUTF16. Some intrinsics, UTF16.getChar(byte[], int) for example, assume the caller performs bounds check (for performance reason). -Sherman On 2/21/18, 11:25 AM, Joe Wang wrote:
Hi Stuart,
Thanks for the apiNote! Joe re-approved the CSR with the added apiNote.
For a test that compares Latin1 vs UTF16 coders, I added tests to the compact string's CompareTo test[1], basically running the original test for String with StringBuilder/Buffer. A build with jdk_core tests passed.
Here's the updated webrev: JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
[1] http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/test/jdk/java/lang/St...
Best, Joe
On 2/14/18, 5:09 PM, Stuart Marks wrote:
Hi Joe,
Overall, looks good.
Are there any tests of AbstractStringBuilder.compareTo() that exercise comparisons of the Latin1 vs UTF16 coders?
A couple people have raised the issue of the SB's implementing Comparable but not overriding equals(). This is unusual but well-defined. I do think it deserves the "inconsistent with equals" treatment. Something like the following should be added to both SB's:
========== @apiNote {@code StringBuilder} implements {@code Comparable} but does not override {@link Object#equals equals}. Thus, the natural ordering of {@code StringBuilder} is inconsistent with equals. Care should be exercised if {@code StringBuilder} objects are used as keys in a {@code SortedMap} or elements in a {@code SortedSet}. See {@link Comparable}, {@link java.util.SortedMap SortedMap}, or {@link java.util.SortedSet SortedSet} for more information. ==========
Respectively for StringBuffer. (Adjust markup to taste.)
Thanks,
s'marks
On 2/8/18 4:47 PM, Joe Wang wrote:
Hi all,
The CSR for the enhancement is now approved. Thanks Joe!
The webrev has been updated accordingly. Please let me know if you have any further comment on the implementation. JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
Thanks, Joe
On 2/2/2018 12:46 PM, Joe Wang wrote:
Thanks Jason. Will update that accordingly.
Best, Joe
On 2/2/2018 11:22 AM, Jason Mehrens wrote:
Joe,
The identity check in CS.compare makes sense. However, it won't be null hostile if we call CS.compare(null, null) and that doesn't seem right. Usually when writing comparator classes I end up with: === if (Objects.requireNonNull(o1) == Objects.requireNonNull(o2)) { return 0; } ===
Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang <huizhe.wang@oracle.com> Sent: Friday, February 2, 2018 1:01 PM To: core-libs-dev Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer
Hi,
Thanks all for comments and suggestions. I've updated the webrev. Please review.
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
Thanks, Joe
On 1/31/2018 9:31 PM, Joe Wang wrote:
Hi Tagir,
Thanks for the comment. I will consider adding that to the javadoc emphasizing that the comparison is performed from 0 to length() - 1 of the two sequences.
Best, Joe
On 1/29/18, 8:07 PM, Tagir Valeev wrote: > Hello! > > An AbstractStringBuilder#compareTo implementation is wrong. You > cannot > simply compare the whole byte array. Here's the test-case: > > public class Test { > public static void main(String[] args) { > StringBuilder sb1 = new StringBuilder("test1"); > StringBuilder sb2 = new StringBuilder("test2"); > sb1.setLength(4); > sb2.setLength(4); > System.out.println(sb1.compareTo(sb2)); > System.out.println(sb1.toString().compareTo(sb2.toString())); > } > } > > We truncated the stringbuilders making their content equal, so > sb1.toString().compareTo(sb2.toString()) is 0, but compareTo > compares > the original content (before the truncation) as truncation, of > course, > does not zero the truncated bytes, neither does it reallocate the > array (unless explicitly asked via trimToSize). > > With best regards, > Tagir Valeev. > > > On Fri, Jan 26, 2018 at 10:00 AM, Joe Wang<huizhe.wang@oracle.com> > wrote: >> Hi, >> >> Adding methods for comparing CharSequence, StringBuilder, and >> StringBuffer. >> >> The Comparable implementations for StringBuilder/Buffer are >> similar >> to that >> of String, allowing comparison operations between two >> StringBuilders/Buffers, e.g. >> aStringBuilder.compareTo(anotherStringBuilder). >> For CharSequence however, refer to the comments in JIRA, a static >> method >> 'compare' is added instead of implementing the Comparable >> interface. >> This >> 'compare' method may take CharSequence implementations such as >> String, >> StringBuilder and StringBuffer, making it possible to perform >> comparison >> among them. The previous example for example is equivalent to >> CharSequence.compare(aStringBuilder, anotherStringBuilder). >> >> Tests for java.base have been independent from each other. The new >> tests are >> therefore created to have no dependency on each other or >> sharing any >> code. >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 >> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/ >> >> Thanks, >> Joe