Hi Andrew,
Sorry to be late to the review.
For symmetry with the constructors StringBuffer(String), and
StringBuilder(String)
the determine the coder based on the input argument, I would recommend
using the getCharSequenceCoder added in the -01 webrev and calling
it from the StringBuffer(CharSeq...), and StringBuilder(CharSeq...)
constructors.
It would be symmetric with the getCoder() method (line 1635)
and select the appropriate coder base on the input value (if known.)
Thanks, Roger
On 03/26/2019 10:57 AM, Andrew Leonard wrote:
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
