Looks good to me!
The copyright years need to be updated.
With kind regards,
Ivan
On 2/26/20 6:12 AM, Claes Redestad wrote:
Hi,
I took the liberty to file
https://bugs.openjdk.java.net/browse/JDK-8240094 for this.
Webrev: http://cr.openjdk.java.net/~redestad/8240094/open.00/
I added a trivial micro and verified that there's a roughly 2.5x
improvement in this targeted test on my machine (~10 ns/op -> ~4ns/op),
while staying neutral on a selection of benchmarks that exercise
newString.
/Claes
On 2020-02-26 14:19, Claes Redestad wrote:
On 2020-02-26 11:01, Сергей Цыпанов wrote:
Currently we have
public static String stripLeading(byte[] value) {
int left = indexOfNonWhitespace(value);
if (left == value.length) {
return "";
}
return (left != 0) ? newString(value, left, value.length - left)
: null;
}
With the patch we change behaviour of this method for the case when
value.length == 0:
public static String stripLeading(byte[] value) {
int left = indexOfNonWhitespace(value);
return (left != 0) ? newString(value, left, value.length - left)
: null;
}
Unlike original method this code returns null instead of "" for
empty array.
This does not affect caller (String.stripLeading()) i. e. visible
behaviour
remains the same for String user, but is it OK in general?
One observable difference on the public API I think will happen here is
that new String("").stripLeading() == "" will change from true to false,
since the null return from the inner method mean we return the argument.
From a compatibility point of view I think this should be fine, as
the identity of the returned empty string isn't specified. I don't think
a CSR is required, but bringing it up since others think otherwise.
Overall I like how the patch now cleans things up a bit on top of
(likely) optimizing a few edge cases, and can volunteer to sponsor it if
no one else has spoken up already.
Doing some quick performance testing and possibly adding a
microbenchmark before push would be good.
Thanks!
/Claes
--
With kind regards,
Ivan Gerasimov
