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

[Remi Forax]

On 06/16/2014 12:22 AM, Claes Redestad wrote:
> I've updated the patch to use CharSequence in favor of String for all 
> new methods, as well as ensuring all new methods are package private 
> (for now):
>
> http://cr.openjdk.java.net/~redestad/8041972/webrev.2/
>
> Reviews appreciated!
>
> /Claes

Thanks, for that,
Rémi

>
> On 2014-06-15 14:29, Claes Redestad wrote:
>>
>> 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
>