Some comments:
- The NumberFormatException.forInputString for some CharSequence is probably
misleading since it doesn't consider the begin-end range which was in effect
for the parsing. Rather than extracting the substring just for the error
message perhaps include the index at which the error occurred. ie. add a
NumberFormatException.forCharSequence(s, idx) static method
- Long.java: Why not throw new NumberFormatException("Empty input") or call
throw NumberFormatException.forInputString("");
- declaration of and assignment to multmin could be combined. declaration of
"result" could also be moved later.
- Since it's not part of the public API I recommend moving the
System/JavaLangAccess changes to a separate RFE. (and it needs a test).
On Jun 27 2014, at 12:02 , Claes Redestad <[email protected]> wrote:
> Hi,
>
> updated webrev:http://cr.openjdk.java.net/~redestad/8041972/webrev.11
>
> Changes:
> - Remove use of IllegalArgumentException in favor of
> IndexOutOfBoundsException/NumberFormatException, making the new methods
> behave in line with how String.substring wouldat some edge cases:
> "100".substring(3)equals "", thus Integer.parseInt("100", 10, 3) now throw
> NumberFormatException, while Integer.parseInt("100", 10,
> 4)/"100".substring(4) will throw IOOB.
> - For performance reasons the old and new methodshave been split apart. This
> introduces some code duplication, but removes the need to add special checks
> in some places.
> - Added more tests
>
> /Claes
>
> On 06/27/2014 10:54 AM, Paul Sandoz wrote:
>> On Jun 26, 2014, at 6:53 PM, Claes Redestad <[email protected]>
>> wrote:
>>
>>> On 06/25/2014 06:43 PM, Paul Sandoz wrote:
>>>> On Jun 19, 2014, at 7:39 PM, Claes Redestad <[email protected]>
>>>> 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.
>>>
>> Ok. Perhaps it might be possible to place the specifics of constraint
>> checking in public methods, but they defer to a general private method that
>> can assume it's arguments are safe (or such arguments are checked by other
>> method calls e.g. CS.charAt)
>>
>>
>>>> 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.
>>>
>> A good point.
>>
>>
>>>> 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. :-)
>>>
>> Right, if we were starting again it might be different but i think
>> consistency is important.
>>
>>
>>>> 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.
>> Doh! what was i thinking. If it stays perhaps place it as a package private
>> method on Number.
>>
>>
>>> If I split the parseX(String... and parseX(CharSequence... implementations
>>> as I intend to, this method will be redundant though.
>>>
>> Ok.
>>
>> Paul.
>
