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

Thu Feb 22 20:04:43 UTC 2018

Hi Sherman,

Thanks for reviewing the change.

Taking a local copy of the count field, but the boundary check would be 
almost immediately done against the field itself. Are you worrying about 
the count field may be out of sync with the byte array? I would think 
that's unlikely to happen. Whether it's StringBuilder or StringBuffer, 
it's not advisable/practical to use in multiple threads. In a valid 
usage, the count is always consistent with the byte array.

Best,
Joe

On 2/21/2018 11:59 AM, Xueming Shen wrote:
> 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
>>>>>
>>>>
>