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.
