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

[Claes Redestad]

On 06/25/2014 06:43 PM, Paul Sandoz wrote:
> On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redestad at oracle.com> wrote:
>> Hi,
>>
>> an updated webrev with reworked, public methods is available here:
>> http://cr.openjdk.java.net/~redestad/8041972/webrev.8/
>>
>> Reviews are yet again appreciated!
>>
> I think "if (s == null)" or "Objects.requireNonNull(s)" is preferable to s.getClass(). (I am informed by those more knowledgeable than i that the latter also has poorer performance in the client compiler.)

Agreed. Using s.getClass() was necessitated to retain performance using 
default compiler (-XX:+TieredCompilation) in the microbenchmarks I've 
been using, and going back to testing with C1 (via means of 
-XX:TieredStartAtLevel=1-3), it's apparent that the patch can cause a 
regression with the client compiler that I hadn't checked.It even looks 
like C2 alone (-XX:-TieredCompilation) suffers slightly.

Changing to Objects.requireNonNull doesn't seem to make things better, 
though. Rather the regression seem to be due to C1 (and in some ways 
even C2) not dealing very well with the increased degrees of freedoms in 
the new methods, so I'm currently considering splitting apart the 
implementations to keep the old implementations of parseX(String[, int]) 
untouched while duplicating some code to build the new methods. Ugly, 
but I guess it's anecessary evil here.

> Related to the above, it might be preferable to retain the existing semantics of throwing NumberFormatException on a null string value, since someone might switch between parseInt(String ) and parseInt(String, int, int, int) and the null will then slip through the catch of NumberFormatException.

Consider Integer.parseInt(s.substring(1)) and Integer.parseInt(s, 10, 
1): the first would throw NullPointerException currently if s == null, 
while the latter instead would start throwing NumberFormatException. I 
think we should favor throwing a NPE here. I'd argue that the risk that 
someone changes to any of the range-based alternatives when they aren't 
replacing a call to substring or similar are slim to none.

>
> You could just use IndexOutOfBoundsException for the case of beginIndex < 0 and beginIndex >= endIndex and endIndex > s.length(), as is similarly the case for String.substring. Again, like previously, switching between parseInt(String, int, int) parseInt(String, int, int, int) requires no additional catching. You might want to add a comment in the code that some IndexOutOfBoundsException exceptions are implicit via calls to s.charAt (i did a double take before realizing :-) ).

Fair points. I could argue String.substring(int, int), 
StringBuilder.append(CharSequence, int, int)etc are wrong, but I guess 
that might be a losing battle. :-)

>
> Integer.requireNonEmpty could be a static package private method on String (or perhaps even static public, it seems useful), plus i would not bother with the static import in this case.

The requireNonEmpty throws NumberFormatException to keep compatibility 
with the old methods, so it wouldn't make much sense to add that to 
String. If I split the parseX(String... and parseX(CharSequence... 
implementations as I intend to, this method will be redundant though.

>
> In Integer.java#643 you can combine the if statements like you have done for the equivalent method on Long.

Will do, thanks!

/Claes
>
> Hth,
> Paul.