RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

Wed Feb 21 19:59:26 UTC 2018

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/String/CompactString/CompareTo.java.sdiff.html
>
> 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 at openjdk.java.net> on 
>>>>> behalf of Joe Wang <huizhe.wang at 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 at 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
>>>>
>>>