On Mon, 15 Mar 2021 14:07:29 GMT, Claes Redestad <[email protected]> wrote:
>> I was thinking about `StringBuilder` at the very beginning but then decided
>> to have no bounds checks and reallocations. Indeed from maintainability
>> point of view your solution is much more attractive. I have one minor
>> comment about `append()`: I think we should do more chaining, e.g.
>> StringBuilder sb = new StringBuilder(len);
>> sb.append(elts[0]);
>> should be
>> StringBuilder sb = new StringBuilder(len).append(elts[0]);
>> etc. to have less bytecode.
>>
>> E.g. if we take
>> public String sb() {
>> StringBuilder sb = new StringBuilder();
>>
>> sb.append("a");
>> sb.append("b");
>>
>> return sb.toString();
>> }
>> `sb.append()` gets compiled into
>> L1
>> LINENUMBER 23 L1
>> ALOAD 1
>> LDC "a"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> L2
>> LINENUMBER 24 L2
>> ALOAD 1
>> LDC "b"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> ```
>> and in case we have
>> ```
>> sb.append("a").append("b");
>> ```
>> the amount of byte code is reduced:
>> ```
>> L1
>> LINENUMBER 23 L1
>> ALOAD 1
>> LDC "a"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> LDC "b"
>> INVOKEVIRTUAL java/lang/StringBuilder.append
>> (Ljava/lang/String;)Ljava/lang/StringBuilder;
>> POP
>> ```
>>
>> Also I'd change
>> if (addLen == 0) {
>> compactElts();
>> return size == 0 ? "" : elts[0];
>> }
>> to
>> if (size == 0) {
>> if (addLen == 0) {
>> return "";
>> }
>> return prefix + suffix;
>> }
>> The second variant is more understandable to me and likely to be slightly
>> faster.
>>
>> And finally, should I close this PR and you'll create a new one from your
>> branch, or I need to copy your changes here?
>
> Up to you. If you adapt your PR to use a StringBuilder as suggested I can
> review and sponsor.
I'll reuse existing PR then.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2627
