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 <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. >