Issue 6354088: Add {Doubles,Floats}.tryParse (Closed)
|
Created:
12 years, 8 months ago by Louis Wasserman Modified:
12 years, 6 months ago CC:
Colin Decker Visibility:
Public. |
Patch Set 1 #
Total comments: 10
Patch Set 2 : Respond to comments #
Total comments: 6
Patch Set 3 : Respond to comments #Patch Set 4 : Add changes to FloatsTest #
Total comments: 8
Patch Set 5 : Respond to comments #
Total comments: 9
Patch Set 6 : Respond to comments #
Total comments: 6
Patch Set 7 : Respond to comments #
Total comments: 8
Patch Set 8 : Respond to comments #
Total comments: 1
Patch Set 9 : Respond to comments #
MessagesTotal messages: 31
Kevin, I'd appreciate it if you could skim the docs; Pete should probably check the tests and implementation. Thanks!
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:548: "(((0[xX](\\p{XDigit}+)(\\.)?)|(0[xX](\\p{XDigit}+)?(\\.)(\\p{XDigit}+)))" + Some formatting issues here, but bigger issues first. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:558: * Valid inputs are exactly those specified by the <i>FloatValue</i> The user's "JDK" may be documented differently. Just say that the same inputs are accepted as in Double.valueOf except for the trim issue. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:565: * @return the integer value represented by {@code string}, or {@code null} if s/integer/{@code double}/ http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:573: if (Doubles.FLOATING_POINT_PATTERN.matcher(string).matches()) { Perhaps comment // TODO(lowasser): this could be optimized in the future, but only with extremely careful testing "Doubles." is unnecessary http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:574: return Double.parseDouble(string); So, if it passed the regex this can't possibly fail, really? Even if so, it may be faster overall to only screen out the most likely failures with the regex and still wrap this in a try/catch?
Sign in to reply to this message.
Am I supposed to be taking a look at this, or waiting for Louis to respond to Kevin's comments? I've been assuming the latter. Pete. On 9 July 2012 13:01, <wasserman.louis@gmail.com> wrote: > Reviewers: kevinb, peteg, > > Message: > Kevin, I'd appreciate it if you could skim the docs; Pete should > probably check the tests and implementation. > > Thanks! > > > > Please review this at http://codereview.appspot.com/**6354088/<http://codereview.appspot.com/6354088/> > > Affected files: > M guava-tests/test/com/google/**common/primitives/DoublesTest.**java > M guava-tests/test/com/google/**common/primitives/FloatsTest.**java > M guava/src/com/google/common/**primitives/Doubles.java > M guava/src/com/google/common/**primitives/Floats.java > > >
Sign in to reply to this message.
If you could survey the tests, that'd be great; I'm getting around to Kevin's comments. (The implementation is fairly predictable.) Louis Wasserman wasserman.louis@gmail.com http://profiles.google.com/wasserman.louis On Wed, Jul 11, 2012 at 6:06 PM, Pete Gillin <peteg@google.com> wrote: > Am I supposed to be taking a look at this, or waiting for Louis to respond > to Kevin's comments? I've been assuming the latter. > > Pete. > > > On 9 July 2012 13:01, <wasserman.louis@gmail.com> wrote: > >> Reviewers: kevinb, peteg, >> >> Message: >> Kevin, I'd appreciate it if you could skim the docs; Pete should >> probably check the tests and implementation. >> >> Thanks! >> >> >> >> Please review this at http://codereview.appspot.com/**6354088/<http://codereview.appspot.com/6354088/> >> >> Affected files: >> M guava-tests/test/com/google/**common/primitives/DoublesTest.**java >> M guava-tests/test/com/google/**common/primitives/FloatsTest.**java >> M guava/src/com/google/common/**primitives/Doubles.java >> M guava/src/com/google/common/**primitives/Floats.java >> >> >> >
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:548: "(((0[xX](\\p{XDigit}+)(\\.)?)|(0[xX](\\p{XDigit}+)?(\\.)(\\p{XDigit}+)))" + On 2012/07/09 14:30:58, kevinb wrote: > Some formatting issues here, but bigger issues first. I broke it up into exactly 80 columns, abandoning attempts to break at parentheses and the like. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:558: * Valid inputs are exactly those specified by the <i>FloatValue</i> On 2012/07/09 14:30:58, kevinb wrote: > The user's "JDK" may be documented differently. Just say that the same inputs > are accepted as in Double.valueOf except for the trim issue. Done. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:565: * @return the integer value represented by {@code string}, or {@code null} if On 2012/07/09 14:30:58, kevinb wrote: > s/integer/{@code double}/ Done. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:573: if (Doubles.FLOATING_POINT_PATTERN.matcher(string).matches()) { On 2012/07/09 14:30:58, kevinb wrote: > Perhaps comment > > // TODO(lowasser): this could be optimized in the future, but only with > extremely careful testing > > "Doubles." is unnecessary Done. http://codereview.appspot.com/6354088/diff/1/guava/src/com/google/common/prim... guava/src/com/google/common/primitives/Doubles.java:574: return Double.parseDouble(string); On 2012/07/09 14:30:58, kevinb wrote: > So, if it passed the regex this can't possibly fail, really? > > Even if so, it may be faster overall to only screen out the most likely failures > with the regex and still wrap this in a try/catch? Fair enough. I'm inclined to leave the regex more or less as-is, but I can get behind having a try/catch anyway.
Sign in to reply to this message.
I think we can be a bit more thorough in the testing. Here are some initial thoughts. http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:407: assertEquals(Double.valueOf(-3.8e16), Doubles.tryParse("-3.8e16")); Other things to try: - type suffix is capital (D or F); - exponent indicator is capital (E); - leading + sign; - leading decimal point (no preceding zero); - type suffix but no exponent; - exponent but no decimal point; - one or more leading zeroes on the exponent; - one or more leading zeroes before a digit (rather than before a decimal point); - probably a whole bunch of other things. Also, potentially, combinations of some of these... I think we could be more systematic about this. http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:449: assertNull(Doubles.tryParse(".e5")); Again, there are probably a bunch more we could be including. The most obvious one would be a plain integer like "5". Also, since your contract makes specific guarantees about rejecting leading and trailing w.s., you should test those. http://codereview.appspot.com/6354088/diff/6001/guava/src/com/google/common/p... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/6001/guava/src/com/google/common/p... guava/src/com/google/common/primitives/Doubles.java:542: * for prevalidating inputs. Should we document explicitly here that it is only required that all valid strings match this pattern, not that all invalid strings don't?
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:407: assertEquals(Double.valueOf(-3.8e16), Doubles.tryParse("-3.8e16")); On 2012/07/12 18:11:24, peteg wrote: > Other things to try: > - type suffix is capital (D or F); > - exponent indicator is capital (E); > - leading + sign; > - leading decimal point (no preceding zero); > - type suffix but no exponent; > - exponent but no decimal point; > - one or more leading zeroes on the exponent; > - one or more leading zeroes before a digit (rather than before a decimal > point); > - probably a whole bunch of other things. > Also, potentially, combinations of some of these... I think we could be more > systematic about this. Is this more systematic? ;) http://codereview.appspot.com/6354088/diff/6001/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:449: assertNull(Doubles.tryParse(".e5")); On 2012/07/12 18:11:24, peteg wrote: > Again, there are probably a bunch more we could be including. The most obvious > one would be a plain integer like "5". Also, since your contract makes specific > guarantees about rejecting leading and trailing w.s., you should test those. Plain integers should pass, *unless* they're in hex. (Why? I really don't know.) In any event, I added a few more here... http://codereview.appspot.com/6354088/diff/6001/guava/src/com/google/common/p... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/6001/guava/src/com/google/common/p... guava/src/com/google/common/primitives/Doubles.java:542: * for prevalidating inputs. On 2012/07/12 18:11:24, peteg wrote: > Should we document explicitly here that it is only required that all valid > strings match this pattern, not that all invalid strings don't? Done.
Sign in to reply to this message.
Yep, I like those tests a lot more! Thanks. Just a few minor points (which obviously apply to floats as well as doubles). Plus a slight boo-boo detected in the floats test... http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:409: } Personally, I think I'd pull this code out into a static method. Partly because you're doing it twice, but mostly because I find this more self-explanatory, you don't have to put in a comment to explain the flow: try { return Double.valueOf(input); } catch (NumberFormatException e) { return null; } http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:427: String input = signChar + hexPrefix + iPart + fPart + expMarker + exponent + typePart; Line length. http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:480: // Double.parseDouble accepts decimal integer inputs, but not hex integers Nit: Slightly confused that the comments appear below the line they refer to. I suggest putting them before, with a colon on the end to make it obvious. (I'd actually be just as happy with below if I could think of a way to make that obvious, but I can't.) http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/FloatsTest.java (right): http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/FloatsTest.java:404: assertEquals(expected, Doubles.tryParse(input)); Ahem. All these doubles should be floats...
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:409: } On 2012/07/13 12:30:02, peteg wrote: > Personally, I think I'd pull this code out into a static method. Partly because > you're doing it twice, but mostly because I find this more self-explanatory, you > don't have to put in a comment to explain the flow: > try { > return Double.valueOf(input); > } catch (NumberFormatException e) { > return null; > } Done. http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:427: String input = signChar + hexPrefix + iPart + fPart + expMarker + exponent + typePart; On 2012/07/13 12:30:02, peteg wrote: > Line length. Done. http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/DoublesTest.java:480: // Double.parseDouble accepts decimal integer inputs, but not hex integers On 2012/07/13 12:30:02, peteg wrote: > Nit: Slightly confused that the comments appear below the line they refer to. I > suggest putting them before, with a colon on the end to make it obvious. (I'd > actually be just as happy with below if I could think of a way to make that > obvious, but I can't.) Done. http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... File guava-tests/test/com/google/common/primitives/FloatsTest.java (right): http://codereview.appspot.com/6354088/diff/1006/guava-tests/test/com/google/c... guava-tests/test/com/google/common/primitives/FloatsTest.java:404: assertEquals(expected, Doubles.tryParse(input)); On 2012/07/13 12:30:02, peteg wrote: > Ahem. All these doubles should be floats... Done.
Sign in to reply to this message.
I have three nits, which apply to DoublesTest and FloatsTest. Otherwise, the tests and the implementations LGTM. Will leave Kevin to comment on API and docs. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:407: return expected; You prefer having a variable which gets assigned or not here, rather than just having a return inside the try and another inside the catch? I'm not going to insist, but I do find the latter clearer. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:417: Nit: Hardly seems worth this blank line now. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:436: Ditto.
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/12001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/12001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:548: "[+-]?(NaN|Infinity|(((\\d+(\\.)?(\\d*)([eE][+-]?\\d+)?)|(\\.(\\d+)(" + style nits: indent 4, not 6, and break before the +s even though it will look grosser that way. http://codereview.appspot.com/6354088/diff/12001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:564: * {@code null} if {@code string} has a length of zero or cannot be parsed as a column overflow, sorry http://codereview.appspot.com/6354088/diff/12001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:581: } else { slight preference to remove this "else" and then not need the "return null" just above.
Sign in to reply to this message.
All nits addressed. After Kevin's LGTM, I'll forward this along to Chris or Colin for import. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... File guava-tests/test/com/google/common/primitives/DoublesTest.java (right): http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:407: return expected; On 2012/07/16 18:51:44, peteg wrote: > You prefer having a variable which gets assigned or not here, rather than just > having a return inside the try and another inside the catch? I'm not going to > insist, but I do find the latter clearer. Done. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:417: On 2012/07/16 18:51:44, peteg wrote: > Nit: Hardly seems worth this blank line now. Done. http://codereview.appspot.com/6354088/diff/12001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/DoublesTest.java:436: On 2012/07/16 18:51:44, peteg wrote: > Ditto. Done.
Sign in to reply to this message.
LGTM I have no idea if there's an official way to do that in this lame-ass code review tool which I have heretofore avoided using.
Sign in to reply to this message.
The frontend seems to recognize that four-letter sequence and turn the comment green, at least... On 19 July 2012 14:50, <kevinb@google.com> wrote: > LGTM > > I have no idea if there's an official way to do that in this lame-ass > code review tool which I have heretofore avoided using. > > http://codereview.appspot.com/**6354088/<http://codereview.appspot.com/6354088/> >
Sign in to reply to this message.
Colin, this has Kevin's and Pete's LGTMs. If this change could be imported from https://code.google.com/r/wassermanlouis-guava/source/detail?r=823d80ebafeea9... , that'd be great; I'll close the issue when the change is mirrored out.
Sign in to reply to this message.
There ought to be a much more extensive test to check for compatibility, that checks all generated strings of a certain length (perhaps runs too long to be a standard part of the test suite). I worry about non-ASCII digits and embedded spaces i'd be happier if the negative tests compared directly with compatibility with Double.parseDouble (except for the whitespace tests) http://codereview.appspot.com/6354088/diff/18001/guava-tests/test/com/google/... File guava-tests/test/com/google/common/primitives/FloatsTest.java (right): http://codereview.appspot.com/6354088/diff/18001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/FloatsTest.java:393: return Float.valueOf(input); Consider trimming the whitespace from input; then the reference implementation and the real implementation should be identical in behaviour. http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:548: "[+-]?(NaN|Infinity|(((\\d+(\\.)?(\\d*)([eE][+-]?\\d+)?)|(\\.(\\d+)(" "[+-]?(NaN|Infinity|(((\\d+(\\.)?(\\d*)([eE][+-]?\\d+)?)|(\\.(\\d+)(" I worry about the construct \\d+(\\.)?(\\d*) which might lead to excessive back-tracking I suggest replacing occurrences of \\d+ with \\d?+ and \\d+(\\.)?(\\d*) with \\d?+(\\.\\d?*)? to create something friendly (possessive quantifiers) for the regex engine.
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Floats.java (right): http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Floats.java:546: * @param string the string representation of a {@code double} value s/double/float/
Sign in to reply to this message.
Hopefully this looks a little better. In particular, I added a test for all strings containing a single Unicode code point -- it takes about 3 seconds, but that's far from the longest test in the core libs suites. http://codereview.appspot.com/6354088/diff/18001/guava-tests/test/com/google/... File guava-tests/test/com/google/common/primitives/FloatsTest.java (right): http://codereview.appspot.com/6354088/diff/18001/guava-tests/test/com/google/... guava-tests/test/com/google/common/primitives/FloatsTest.java:393: return Float.valueOf(input); On 2012/07/20 20:51:33, martinrb wrote: > Consider trimming the whitespace from input; then the reference implementation > and the real implementation should be identical in behaviour. We've ruled out allowing whitespace in Doubles/Floats.tryParse. (Kevin's exact words were "hell no...") Unless you mean checking for whitespace in the reference implementation, in which case, done. http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:548: "[+-]?(NaN|Infinity|(((\\d+(\\.)?(\\d*)([eE][+-]?\\d+)?)|(\\.(\\d+)(" On 2012/07/20 20:51:33, martinrb wrote: > "[+-]?(NaN|Infinity|(((\\d+(\\.)?(\\d*)([eE][+-]?\\d+)?)|(\\.(\\d+)(" > I worry about the construct > \\d+(\\.)?(\\d*) > which might lead to excessive back-tracking > I suggest replacing occurrences of \\d+ with \\d?+ > and > \\d+(\\.)?(\\d*) > with > \\d?+(\\.\\d?*)? > to create something friendly (possessive quantifiers) for the regex engine. I had problems with that approach -- the doc says \\d?+ accepts one or zero possessively, I'm not sure that's what you meant -- but here's something that does pass tests. http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Floats.java (right): http://codereview.appspot.com/6354088/diff/18001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Floats.java:546: * @param string the string representation of a {@code double} value On 2012/07/20 21:48:59, martinrb wrote: > s/double/float/ Done.
Sign in to reply to this message.
I tried (and failed) to break this, by creating my own test method. This method
is heretical by using Random non-deterministically and consuming far too much
CPU time, but I like it, and it raises my own confidence that your
implementation is correct.
public void testTryParseMartin() {
String[] fragments = {
"x", "X", "f", "F", "d", "D", "p", "P",
"0", "3", "99",
".", "+", "-", "FOO",
"Infinity", "INFINITY", "infinity",
"NaN", "nan", "NAN",
"\u0001", " ",
};
java.util.Random rnd = new java.util.Random();
StringBuilder sb = new StringBuilder();
for (int len = 0; len < 8; len++) {
for (int iterations = 0; iterations < 1000000; iterations++) {
sb.setLength(0);
for (int i = 0; i < len; i++) {
sb.append(fragments[rnd.nextInt(fragments.length)]);
}
String input = sb.toString();
assertEquals(referenceTryParse(input), Floats.tryParse(input));
}
}
}
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/...
File guava/src/com/google/common/primitives/Doubles.java (right):
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/...
guava/src/com/google/common/primitives/Doubles.java:551: String decimal =
"(\\d++(\\.\\d*+)?|\\.\\d++)";
I suggest making all of the groupings non-capturing, i.e.
change all ( to (?:
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/...
guava/src/com/google/common/primitives/Doubles.java:555: String fpPattern =
"[+-]?(NaN|Infinity|" + completeDec + "|" + completeHex + ")[fFdD]?";
It looks to me like this accepts the string "NaNd", unlike the regex in
Double#valueOf(String). That would be a bug, except that false positives are
OK.
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/...
guava/src/com/google/common/primitives/Doubles.java:584: // GWT, Android may
have somewhat different parameters, so we catch
I think we need this for all javaoid environments, so I would just delete this
comment (unless I misunderstand it)
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:564: * <p>Unlike {@link Double#parseDouble(String)}, this method returns I think we should try to half-promise better performance when failures are common. """This method is likely to have better performance than parseDouble when parse failures are common."""
Sign in to reply to this message.
Since we fallback to Double.parseDouble, false positive bugs in our regexp detection (which only cause performance regression, but should nevertheless be fixed) will not be found unless we include white box test of the regex fpPattern itself, which I would do. And we have such a (small) bug.
Sign in to reply to this message.
http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Doubles.java (right): http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:551: String decimal = "(\\d++(\\.\\d*+)?|\\.\\d++)"; On 2012/07/23 21:13:38, martinrb wrote: > I suggest making all of the groupings non-capturing, i.e. > change all ( to (?: Done. http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:555: String fpPattern = "[+-]?(NaN|Infinity|" + completeDec + "|" + completeHex + ")[fFdD]?"; On 2012/07/23 21:13:38, martinrb wrote: > It looks to me like this accepts the string "NaNd", unlike the regex in > Double#valueOf(String). That would be a bug, except that false positives are > OK. Done. http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:564: * <p>Unlike {@link Double#parseDouble(String)}, this method returns On 2012/07/23 21:20:29, martinrb wrote: > I think we should try to half-promise better performance when failures are > common. """This method is likely to have better performance than parseDouble > when parse failures are common.""" Done. http://codereview.appspot.com/6354088/diff/25001/guava/src/com/google/common/... guava/src/com/google/common/primitives/Doubles.java:584: // GWT, Android may have somewhat different parameters, so we catch On 2012/07/23 21:13:38, martinrb wrote: > I think we need this for all javaoid environments, so I would just delete this > comment (unless I misunderstand it) Edited some.
Sign in to reply to this message.
Looks good. Thank you for your patience. Not all of the changes to Double* have been propagated to corresponding Float* files - that should be done before commit. Hmmm... I'm having a hard time finding the LGTM button in Rietveld. http://codereview.appspot.com/6354088/diff/31002/guava/src/com/google/common/... File guava/src/com/google/common/primitives/Floats.java (right): http://codereview.appspot.com/6354088/diff/31002/guava/src/com/google/common/... guava/src/com/google/common/primitives/Floats.java:561: // GWT, Android may have somewhat different parameters, so we catch Align this comment with the one in Doubles.
Sign in to reply to this message.
Martin: Including the magic word 'LGTM' in your comment makes it turn green in the UI. I think that's about as close as it gets to a button. On 2012/07/26 22:46:47, martinrb wrote: > Looks good. Thank you for your patience. > > Not all of the changes to Double* have been propagated to corresponding Float* > files - that should be done before commit. > > Hmmm... I'm having a hard time finding the LGTM button in Rietveld. > > http://codereview.appspot.com/6354088/diff/31002/guava/src/com/google/common/... > File guava/src/com/google/common/primitives/Floats.java (right): > > http://codereview.appspot.com/6354088/diff/31002/guava/src/com/google/common/... > guava/src/com/google/common/primitives/Floats.java:561: // GWT, Android may have > somewhat different parameters, so we catch > Align this comment with the one in Doubles.
Sign in to reply to this message.
On Fri, Jul 27, 2012 at 1:35 AM, <peteg@google.com> wrote: > Martin: Including the magic word 'LGTM' in your comment makes it turn > green in the UI. I think that's about as close as it gets to a button. > > Thanks. Amazing. Maybe they could make the UI of the reviewer get progressively greener as they are typing the characters L...G...T...M ?!
Sign in to reply to this message.
I wasn't actually quite sure which of these changes should be mirrored to Floats, since it's testing a constant defined only in Doubles... Louis Wasserman wasserman.louis@gmail.com http://profiles.google.com/wasserman.louis On Fri, Jul 27, 2012 at 8:49 PM, Martin Buchholz <martinrb@google.com>wrote: > > > On Fri, Jul 27, 2012 at 1:35 AM, <peteg@google.com> wrote: > >> Martin: Including the magic word 'LGTM' in your comment makes it turn >> green in the UI. I think that's about as close as it gets to a button. >> >> Thanks. Amazing. > > Maybe they could make the UI of the reviewer get progressively greener as > they are typing the characters L...G...T...M ?! >
Sign in to reply to this message.
On Fri, Jul 27, 2012 at 1:15 PM, Louis Wasserman <wasserman.louis@gmail.com>wrote: > I wasn't actually quite sure which of these changes should be mirrored to > Floats, since it's testing a constant defined only in Doubles... > > Good point. ==> LGTM <==
Sign in to reply to this message.
On Fri, Jul 27, 2012 at 1:15 PM, Louis Wasserman <wasserman.louis@gmail.com>wrote: > I wasn't actually quite sure which of these changes should be mirrored to > Floats, since it's testing a constant defined only in Doubles... > > Hmmm... Does Rietveld have a comment-quoting bug? ==> LGTM <==
Sign in to reply to this message.
Maybe it's only comments which _start_ with LGTM? Louis Wasserman wasserman.louis@gmail.com http://profiles.google.com/wasserman.louis On Fri, Jul 27, 2012 at 9:55 PM, Martin Buchholz <martinrb@google.com>wrote: > > > On Fri, Jul 27, 2012 at 1:15 PM, Louis Wasserman < > wasserman.louis@gmail.com> wrote: > >> I wasn't actually quite sure which of these changes should be mirrored to >> Floats, since it's testing a constant defined only in Doubles... >> >> > Hmmm... Does Rietveld have a comment-quoting bug? ==> LGTM <== >
Sign in to reply to this message.
On Fri, Jul 27, 2012 at 1:58 PM, Louis Wasserman <wasserman.louis@gmail.com>wrote: > Maybe it's only comments which _start_ with LGTM? No. See which messages appear green at http://codereview.appspot.com/6354088/
Sign in to reply to this message.
I've mirrored the tests to FloatsTest, but excised the regex-testing parts. Colin, could you mirror this in from https://code.google.com/r/wassermanlouis-guava/source/list?name=try-parse-fp ?
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
