Hi Ivan, Yes, i'm happy with that, as you say the simple constructor change fixes the immediate issue, but not necessarily the extended issue of a non-compactable CharSequence in COMPACT_STRINGS mode, but that's probably an enhanced issue to cover in a separate bug... I've created a new webrev.02 with just the constructor change and the testcase: http://cr.openjdk.java.net/~aleonard/8221430/webrev.02/
Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: [email protected] From: Ivan Gerasimov <[email protected]> To: Andrew Leonard <[email protected]> Cc: [email protected] Date: 26/03/2019 01:18 Subject: Re: Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified Thanks Andrew! Introducing getCharSequenceCoder() is actually an enhancement, which may improve pre-allocation in certain cases. It's not actually required to restore correctness of StringBuilder/Buffer constructors. I recommend to separate it from this bug fix, so it can be discussed separately, and determined if this is the best approach to this enhancement. If you agree, I can adjust your latest patch accordingly, run it through the Mach5 test systems and push it on your behalf. With kind regards, Ivan On 3/25/19 5:00 PM, Andrew Leonard wrote: Hi Ivan, Here is my updated webrev: http://cr.openjdk.java.net/~aleonard/8221430/webrev.01/ Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: [email protected] From: Ivan Gerasimov <[email protected]> To: Andrew Leonard <[email protected]> Cc: [email protected] Date: 25/03/2019 22:55 Subject: Re: Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified I was thinking of organizing code similar to what is done in AbstractStringBuilder(int): if (COMPACT_STRINGS && coderHint == LATIN1) { value = new byte[capacity]; coder = LATIN1; } else { value = StringUTF16.newBytesFor(capacity); coder = UTF16; } With kind regards, Ivan On 3/25/19 3:45 PM, Andrew Leonard wrote: Hi Ivan, I think I see what you're saying, you mean we also need to correct this line in AbstractStringBuilder constructor: value = (coder == LATIN1) ? new byte[capacity] : StringUTF16.newBytesFor(capacity); to be maybe: value = (COMPACT_STRINGS && coder == LATIN1) ? new byte[capacity] : StringUTF16.newBytesFor(capacity); The passed in coder stills need to be correct, since with COMPACT_STRINGS a string could be UTF16 if it cannot be compacted, so it's more than just a hint isn't it? Thanks Andrew Andrew Leonard Java Runtimes Development IBM Hursley IBM United Kingdom Ltd Phone internal: 245913, external: 01962 815913 internet email: [email protected] From: Ivan Gerasimov <[email protected]> To: Andrew Leonard <[email protected]>, [email protected] Date: 25/03/2019 22:20 Subject: Re: Request for sponsor: JDK-8221430: StringBuffer(CharSequence) constructor truncates when -XX:-CompactStrings specified Hi Andrew! Thanks for finding this bug! Your fix solves the problem. However, I think the main issue is that the constructor AbstractStringBuilder(byte,int,int) is now broken: as you discovered, it allows to create a string buffer with the coder LATIN1 when COMPACT_STRINGS is false. Wouldn't it make sense to rename the argument of the constructor to, say, coderHint, and then either use it as the coder if COMPACT_STRINGS==true, or discard it otherwise. What do you think? With kind regards, Ivan On 3/25/19 12:45 PM, Andrew Leonard wrote: > Hi, > Please can I request a sponsor for this fix to a JDK-13 regression? > > Patch with jtreg testcase here: > http://cr.openjdk.java.net/~aleonard/8221430/webrev.00/ > > bug: https://bugs.openjdk.java.net/browse/JDK-8221430 > > Many thanks > Andrew > > Andrew Leonard > Java Runtimes Development > IBM Hursley > IBM United Kingdom Ltd > Phone internal: 245913, external: 01962 815913 > internet email: [email protected] > > Unless stated otherwise above: > IBM United Kingdom Limited - Registered in England and Wales with number > 741598. > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU > -- With kind regards, Ivan Gerasimov Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU -- With kind regards, Ivan Gerasimov Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU -- With kind regards, Ivan Gerasimov Unless stated otherwise above: IBM United Kingdom Limited - Registered in England and Wales with number 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
