RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer
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
Joe, 1. Seems odd that StringBuilder and StringBuffer are comparable but don't have an equals/hashcode implementation. Seems like if you are to stick with that you'll have to add the standard "natural ordering is inconsistent with equals" verbiage. 2. For StringBuffer compare, won't you have to either lock the 'another' object too so it is not mutated or perform some sort of copy before compare? Neither sound like a good option. 3. Does CharSequence.compare need to specify that IndexOutOfBoundsException may be thrown under concurrent modification? 4. For CharSequence.compare, instead of creating a special case for string would it make sense to do: ==== if (cs1.getClass() == cs2.getClass() && cs1 instanceof Comparable) { return ((Comparable<Object>) cs1).compareTo(cs2); } ==== Given #1 and #2 maybe StringBuilder and StringBuffer shouldn't implement comparable and just rely on users either calling 'sb1.toString().compare(sb2.toString())' or 'CharSequence.compare(sb1, sb2)'. Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang <huizhe.wang@oracle.com> Sent: Thursday, January 25, 2018 9:00 PM To: core-libs-dev Subject: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer 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
Thanks for the comments, Jason. The topic about equals/hashcode had been discussed. They are currently inherited, and by definition of CharSequence, equality is generally undefined. One could argue for it to be a lexicographic comparison, but that means changing the current specification, potentially breaking compatibility. Note that we made subtle wording change to the general description of CharSequence. For #2, I'll add a clarification in the spec similar to those that deal with appending a CharSequence or StringBuffer, that the method does not synchronize on the source. A better concurrent lock mechanism could have been devised for StringBuffer, but it'd be a fruitless effort to attempt to change its poor state. Given the nature of these classes, in almost all cases, StringBuilder is recommended over StringBuffer. So our effort here is to maintain a consistency between the classes. #3, Concurrency would be better handled outside of the compare method (same as the above). #4, sounds good. I'll look into the modification and probably add some more test cases. Best, Joe On 1/26/2018 6:23 AM, Jason Mehrens wrote:
Joe,
1. Seems odd that StringBuilder and StringBuffer are comparable but don't have an equals/hashcode implementation. Seems like if you are to stick with that you'll have to add the standard "natural ordering is inconsistent with equals" verbiage. 2. For StringBuffer compare, won't you have to either lock the 'another' object too so it is not mutated or perform some sort of copy before compare? Neither sound like a good option. 3. Does CharSequence.compare need to specify that IndexOutOfBoundsException may be thrown under concurrent modification? 4. For CharSequence.compare, instead of creating a special case for string would it make sense to do:
==== if (cs1.getClass() == cs2.getClass() && cs1 instanceof Comparable) { return ((Comparable<Object>) cs1).compareTo(cs2); } ====
Given #1 and #2 maybe StringBuilder and StringBuffer shouldn't implement comparable and just rely on users either calling 'sb1.toString().compare(sb2.toString())' or 'CharSequence.compare(sb1, sb2)'.
Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang <huizhe.wang@oracle.com> Sent: Thursday, January 25, 2018 9:00 PM To: core-libs-dev Subject: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer
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
Just to note that JSR-310 had to add a CharSequence comparison method. Not the same as this one, but a data point, and perhaps a target to become public in a future webrev. See DateTimeParseContext.subSequenceEquals(). Stephen On 26 January 2018 at 03:00, 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
Thanks Stephen for the note. I downloaded the api [1], but don't seem to see a class "DateTimeParseContext". Do you have a pointer to the current webrev? What does it do, and what would be the implication with regards to the CharSequence compare method? [1] https://jcp.org/en/jsr/detail?id=310 Best, Joe On 1/26/2018 7:01 AM, Stephen Colebourne wrote:
Just to note that JSR-310 had to add a CharSequence comparison method. Not the same as this one, but a data point, and perhaps a target to become public in a future webrev. See DateTimeParseContext.subSequenceEquals().
Stephen
On 26 January 2018 at 03:00, 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
java.time.format.DateTimeParseContext.subSequenceEquals On 1/26/18, 1:48 PM, huizhe wang wrote:
Thanks Stephen for the note.
I downloaded the api [1], but don't seem to see a class "DateTimeParseContext". Do you have a pointer to the current webrev? What does it do, and what would be the implication with regards to the CharSequence compare method?
[1] https://jcp.org/en/jsr/detail?id=310
Best, Joe
On 1/26/2018 7:01 AM, Stephen Colebourne wrote:
Just to note that JSR-310 had to add a CharSequence comparison method. Not the same as this one, but a data point, and perhaps a target to become public in a future webrev. See DateTimeParseContext.subSequenceEquals().
Stephen
On 26 January 2018 at 03:00, 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
Thanks Sherman. Found it now. Interesting method. What was the motivation? Does any class implement CharSequence in 310? Best, Joe On 1/26/2018 2:12 PM, Xueming Shen wrote:
java.time.format.DateTimeParseContext.subSequenceEquals
On 1/26/18, 1:48 PM, huizhe wang wrote:
Thanks Stephen for the note.
I downloaded the api [1], but don't seem to see a class "DateTimeParseContext". Do you have a pointer to the current webrev? What does it do, and what would be the implication with regards to the CharSequence compare method?
[1] https://jcp.org/en/jsr/detail?id=310
Best, Joe
On 1/26/2018 7:01 AM, Stephen Colebourne wrote:
Just to note that JSR-310 had to add a CharSequence comparison method. Not the same as this one, but a data point, and perhaps a target to become public in a future webrev. See DateTimeParseContext.subSequenceEquals().
Stephen
On 26 January 2018 at 03:00, 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
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
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
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
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
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
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
Hi Joe, Looking good, but a few comments: AbstractStringBuilder: 111 the coder() method should be private and since there is only a few uses the function could be inlined. StringBuffer:192: extra leading space before "}" Thanks, Roger On 2/8/2018 7: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
On 2/9/18, 1:38 PM, Roger Riggs wrote:
Hi Joe,
Looking good, but a few comments:
AbstractStringBuilder: 111 the coder() method should be private and since there is only a few uses the function could be inlined.
Indeed, the coder() method was added along with the new method. The coder field was referenced directly in all existing uses. I've removed the coder() method, and instead refer to the coder field directly.
StringBuffer:192: extra leading space before "}"
Fixed. Thanks, Joe
Thanks, Roger
On 2/8/2018 7: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
Joe, In CharSequence, does it make sense to cache the result of the length calculation? As in change: ==== for (int i = 0; i < Math.min(cs1.length(), cs2.length()); i++) { ==== for (int i = 0, len = Math.min(cs1.length(), cs2.length()); i < len; i++) { ==== Jason ________________________________________ From: core-libs-dev <core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang <huizhe.wang@oracle.com> Sent: Thursday, February 8, 2018 6:47 PM To: core-libs-dev Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer 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
Done. Thanks Jason! Joe On 2/9/18, 1:46 PM, Jason Mehrens wrote:
Joe, In CharSequence, does it make sense to cache the result of the length calculation? As in change: ==== for (int i = 0; i< Math.min(cs1.length(), cs2.length()); i++) { ==== for (int i = 0, len = Math.min(cs1.length(), cs2.length()); i< len; i++) { ====
Jason ________________________________________ From: core-libs-dev<core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang<huizhe.wang@oracle.com> Sent: Thursday, February 8, 2018 6:47 PM To: core-libs-dev Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer
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
Looks good. Jason ________________________________________ From: Joe Wang <huizhe.wang@oracle.com> Sent: Monday, February 12, 2018 12:25 PM To: Jason Mehrens Cc: core-libs-dev Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer Done. Thanks Jason! Joe On 2/9/18, 1:46 PM, Jason Mehrens wrote:
Joe, In CharSequence, does it make sense to cache the result of the length calculation? As in change: ==== for (int i = 0; i< Math.min(cs1.length(), cs2.length()); i++) { ==== for (int i = 0, len = Math.min(cs1.length(), cs2.length()); i< len; i++) { ====
Jason ________________________________________ From: core-libs-dev<core-libs-dev-bounces@openjdk.java.net> on behalf of Joe Wang<huizhe.wang@oracle.com> Sent: Thursday, February 8, 2018 6:47 PM To: core-libs-dev Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer
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
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
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
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
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/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
On 2/22/18, 12:04 PM, Joe Wang wrote:
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.
Hi Joe, It might not be a "valid usage" but it did happen and when it happens it might just crash the vm without those boundary checks. It's especially true for those intrinsics methods with explicit comments "intrinsic performs no bounds checks". In this case, the StringUTF16.getChar() is being called in new public method StringUTF16.compareTo(byte[], byte[], int, int) without appropriate boundary check. In the "old" code the "index" is guaranteed to be within [0, len) in StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for such scenario can be found in JDK-8158168 [1], for example. -Sherman [1] https://bugs.openjdk.java.net/browse/JDK-8158168
On 2/22/18, 12:51 PM, Xueming Shen wrote:
On 2/22/18, 12:04 PM, Joe Wang wrote:
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.
Hi Joe,
It might not be a "valid usage" but it did happen and when it happens it might just crash the vm without those boundary checks. It's especially true for those intrinsics methods with explicit comments "intrinsic performs no bounds checks". In this case, the StringUTF16.getChar() is being called in new public method StringUTF16.compareTo(byte[], byte[], int, int) without appropriate boundary check. In the "old" code the "index" is guaranteed to be within [0, len) in StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for such scenario can be found in JDK-8158168 [1], for example.
Thanks for the pointer! The email thread helps a lot. I've updated the webrev with a boundary check in ASB (AbstractStringBuilder line 106, 107), and then a note to StringUTF16.compareTo (StringUTF16 line 280). Hopefully this is sufficient. Didn't want to add any check in StringUTF16 since that may affect the original two-arg method. JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/ -Joe
-Sherman
Hi Sherman and all, Thanks for the further reviews! Here's the latest webrev with boundary checks in StringLatin1/StringUTF16: JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/ Best, Joe On 2/22/2018 6:07 PM, Joe Wang wrote:
On 2/22/18, 12:51 PM, Xueming Shen wrote:
On 2/22/18, 12:04 PM, Joe Wang wrote:
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.
Hi Joe,
It might not be a "valid usage" but it did happen and when it happens it might just crash the vm without those boundary checks. It's especially true for those intrinsics methods with explicit comments "intrinsic performs no bounds checks". In this case, the StringUTF16.getChar() is being called in new public method StringUTF16.compareTo(byte[], byte[], int, int) without appropriate boundary check. In the "old" code the "index" is guaranteed to be within [0, len) in StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for such scenario can be found in JDK-8158168 [1], for example.
Thanks for the pointer! The email thread helps a lot. I've updated the webrev with a boundary check in ASB (AbstractStringBuilder line 106, 107), and then a note to StringUTF16.compareTo (StringUTF16 line 280). Hopefully this is sufficient. Didn't want to add any check in StringUTF16 since that may affect the original two-arg method.
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
-Joe
-Sherman
+1 On 2/27/18, 11:37 AM, Joe Wang wrote:
Hi Sherman and all,
Thanks for the further reviews!
Here's the latest webrev with boundary checks in StringLatin1/StringUTF16:
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
Best, Joe
On 2/22/2018 6:07 PM, Joe Wang wrote:
On 2/22/18, 12:51 PM, Xueming Shen wrote:
On 2/22/18, 12:04 PM, Joe Wang wrote:
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.
Hi Joe,
It might not be a "valid usage" but it did happen and when it happens it might just crash the vm without those boundary checks. It's especially true for those intrinsics methods with explicit comments "intrinsic performs no bounds checks". In this case, the StringUTF16.getChar() is being called in new public method StringUTF16.compareTo(byte[], byte[], int, int) without appropriate boundary check. In the "old" code the "index" is guaranteed to be within [0, len) in StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for such scenario can be found in JDK-8158168 [1], for example.
Thanks for the pointer! The email thread helps a lot. I've updated the webrev with a boundary check in ASB (AbstractStringBuilder line 106, 107), and then a note to StringUTF16.compareTo (StringUTF16 line 280). Hopefully this is sufficient. Didn't want to add any check in StringUTF16 since that may affect the original two-arg method.
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
-Joe
-Sherman
+1; looks good On 2/27/2018 3:15 PM, Xueming Shen wrote:
+1
On 2/27/18, 11:37 AM, Joe Wang wrote:
Hi Sherman and all,
Thanks for the further reviews!
Here's the latest webrev with boundary checks in StringLatin1/StringUTF16:
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
Best, Joe
On 2/22/2018 6:07 PM, Joe Wang wrote:
On 2/22/18, 12:51 PM, Xueming Shen wrote:
On 2/22/18, 12:04 PM, Joe Wang wrote:
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.
Hi Joe,
It might not be a "valid usage" but it did happen and when it happens it might just crash the vm without those boundary checks. It's especially true for those intrinsics methods with explicit comments "intrinsic performs no bounds checks". In this case, the StringUTF16.getChar() is being called in new public method StringUTF16.compareTo(byte[], byte[], int, int) without appropriate boundary check. In the "old" code the "index" is guaranteed to be within [0, len) in StringUTF16.compareTo(byte[], byte[]), so it's safe. A real case for such scenario can be found in JDK-8158168 [1], for example.
Thanks for the pointer! The email thread helps a lot. I've updated the webrev with a boundary check in ASB (AbstractStringBuilder line 106, 107), and then a note to StringUTF16.compareTo (StringUTF16 line 280). Hopefully this is sufficient. Didn't want to add any check in StringUTF16 since that may affect the original two-arg method.
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
-Joe
-Sherman
participants (8)
-
huizhe wang
-
Jason Mehrens
-
Joe Wang
-
Roger Riggs
-
Stephen Colebourne
-
Stuart Marks
-
Tagir Valeev
-
Xueming Shen