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

[Joe Wang]

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
>>>
>>