Thank you for looking at this, Mike!

 New webrev: http://cr.openjdk.java.net/~redestad/8041972/webrev.12

On 2014-07-12 00:55, Mike Duigou wrote:
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

I think not extracting the actual substring for error messages will quickly become awkward for cases where the sequence is representing something really large and/or noisy, so I think it's safer to only output the subsequence being parsed. In line with your other suggestions, I've added a forCharSequence which takes the sequence, beginIndex, endIndex and the index at which the error was generated to produce NFEs with messages in the style of 'Error at index 5 in: "-1234S"'


- Long.java: Why not throw new NumberFormatException("Empty input") or call throw 
NumberFormatException.forInputString("");
Fixed!

- declaration of and assignment to multmin could be combined. declaration of 
"result" could also be moved later.
Fixed. I did the same for digit and narrowed the scoping of these vars.

- 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).
I've broken out those changes and will file a separate RFE for the formatUnsigned additions to JLS. Do you think we need to drop the "/format" from the name of this RFE?

Thanks!

/Claes


On Jun 27 2014, at 12:02 , Claes Redestad <claes.redes...@oracle.com> 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 <claes.redes...@oracle.com> wrote:

On 06/25/2014 06:43 PM, Paul Sandoz wrote:
On Jun 19, 2014, at 7:39 PM, Claes Redestad <claes.redes...@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.

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.

Reply via email to