> On 29 Jul 2016, at 05:15, Claes Redestad <claes.redes...@oracle.com> wrote: > > Hi Paul, > > On 07/28/2016 02:55 PM, Paul Sandoz wrote: >>> On 27 Jul 2016, at 19:36, Claes Redestad <claes.redes...@oracle.com> wrote: >>> >>> On 07/25/2016 08:01 PM, Iris Clark wrote: >>>> Hi, Claes. >>>> >>>>> Webrev: http://cr.openjdk.java.net/~redestad/8162439/webrev.01/ >>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8162439 >>>> I think that this change looks good. We provide a shortcut for the common >>>> case where only the major version number is of interest without >>>> introducing a new API. >>> Thanks! Any reviewer want to give this the go-ahead? >> Looks ok. >> >> I suppose you could do: >> >> boolean isSimpleNumber(String s) { >> for (int i = 0; i < s.length(); i++) { >> char c = s.get(i); >> if (c < ((i > 0) ? ‘0’ : ‘1’) || c > ‘9’) >> return false; >> } >> return true; >> } >> >> >> if (isSimpleNumber(s)) { >> ... >> } else { >> return VersionBuilder.parse(s); >> } >> >> then let VersionBuilder.parse throw errors in incorrectly formatted version >> strings. > > sounds reasonable. It'd still change the behavior for the empty string from > IAE to NFE, but only having to do one pass over the input string is nice. >
Hmm… i am inclined to agree because i am not sure given the pattern matching that one can reliably produce an NFE as anticipated in the JavaDoc. > Another reasonable comment I got offline was that this patch splits the > parsing into two places, which is hacky. Since what we really want to avoid > is to eagerly load the Version string patterns (pulling in large parts of > j.u.regex), we could inline the parsing code into Runtime.Version.parse, > rename VersionBuilder to VersionPattern and access the constants therein from > the parse method to allow lazy initialization. The overall result is arguably > cleaner: > > http://cr.openjdk.java.net/~redestad/8162439/webrev.02/ > Yes, much better. I was thinking similar thoughts about the split after i sent my previous email. Paul.
