RFR [9] 8041972: Add improved parse/format methods for Long/Integer

[Claes Redestad]

On 2014-06-15 13:48, Remi Forax wrote:
>
> On 06/15/2014 12:36 AM, Claes Redestad wrote:
>> Hi,
>>
>> please review this patch to add offset based variants of 
>> Integer.parseInt/parseUnsignedInt, Long.parseLong/parseUnsignedLong 
>> and expose them through JavaLangAccess along with 
>> formatUnsignedInt/-Long. This is proposed to enable a number of minor 
>> optimizations, starting with 
>> https://bugs.openjdk.java.net/browse/JDK-8006627
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8041972/webrev.0/
>>
>> Thanks!
>>
>> /Claes
>
> In my opinion, and as suggested in the description of 8041972, these 
> methods should be public.
Ultimately, yes. The ulterior motive here is to be able to backport 
these to 8u40 (maybe even 7) for internal JDK use, while updating to 
public (and dropping the SharedSecrets) should be done in a later, 
9-only update. Adding public methods would require CCC approval, more 
detailed javadocs and possibly be part of a JEP, so pushing a 
back-portable internal patch as a first step seems reasonable.
> Having written my share of scanners/parsers in Java, these methods are 
> really helpful but
> given that they only rely on charAt/length of String, I think they 
> should take a CharSequence and not a String as first parameter,
> in order to support other storages of characters like by example a 
> java.nio.CharBuffer.
>
> So for me, instead of adding a bunch of method in shared secrets, 
> those method should be public and take a CharSequence.
I wouldn't mind switching over to CharSequence for the newly added 
methods. I wasn't around last time[1] this was proposed for the existing 
methods, but I know there were arguments that changing to a (possibly) 
mutable input type might have problematic side effects that would form a 
valid argument against this switch, while I personally share the opinion 
that it's up to the caller to enforce immutability when necessary and 
that text parsing methods should all use CharSequence when possible.
>
> Miinor nits, Integer.parseInt(String,int,int) don't need to test if s 
> is null given it delegated to parseInt(String, int, int, int),
> and Integer.parseInt(String) should directly call parseInt(s, 10, 0, 
> s.length()) instead of calling parseInt(s, 10) that calls
> parseInt(s, 10, 0) that calls parseInt(s, 10, 0, s.length()).
>
> cheers,
> Rémi
Not checking for null in parseInt(String, int, int) would mean we would 
get a NPE calling s.length() in the call to parseInt(String, int, int, 
int), so this is needed for compatibility. Microbenchmarks suggests the 
extra check does not have any performance impact since the JIT can 
easily prove that the inner null checks can be removed when an outer method.

Calling parseInt(s, 10, 0, s.length()) directly in place of parseInt(s, 
10, 0) would likewise require an earlier null check (slightly more code 
duplication) while being performance neutral either way.

/Claes

[1] 
https://www.mail-archive.com/core-libs-dev@openjdk.java.net/msg09694.html