]> Cypherpunks repositories - gostls13.git/commitdiff
strconv: add comment re extFloat errorscale
authorNigel Tao <nigeltao@golang.org>
Fri, 10 Apr 2020 01:01:14 +0000 (11:01 +1000)
committerNigel Tao <nigeltao@golang.org>
Sat, 11 Apr 2020 23:08:34 +0000 (23:08 +0000)
Change-Id: I6f006ba72e1711ba2a24cd71552855ad88284eec
Reviewed-on: https://go-review.googlesource.com/c/go/+/227797
Reviewed-by: Rémy Oudompheng <remyoudompheng@gmail.com>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
src/strconv/extfloat.go

index 2a2dd7a4084457f18bdf4cf5e3cf6a8e78b990dd..793a34d83fb8e07798d22a62977690f060824797 100644 (file)
@@ -231,8 +231,30 @@ var uint64pow10 = [...]uint64{
 // float32 depending on flt.
 func (f *extFloat) AssignDecimal(mantissa uint64, exp10 int, neg bool, trunc bool, flt *floatInfo) (ok bool) {
        const uint64digits = 19
+
+       // Errors (in the "numerical approximation" sense, not the "Go's error
+       // type" sense) in this function are measured as multiples of 1/8 of a ULP,
+       // so that "1/2 of a ULP" can be represented in integer arithmetic.
+       //
+       // The C++ double-conversion library also uses this 8x scaling factor:
+       // https://github.com/google/double-conversion/blob/f4cb2384/double-conversion/strtod.cc#L291
+       // but this Go implementation has a bug, where it forgets to scale other
+       // calculations (further below in this function) by the same number. The
+       // C++ implementation does not forget:
+       // https://github.com/google/double-conversion/blob/f4cb2384/double-conversion/strtod.cc#L366
+       //
+       // Scaling the "errors" in the "is mant_extra in the range (halfway ±
+       // errors)" check, but not scaling the other values, means that we return
+       // ok=false (and fall back to a slower atof code path) more often than we
+       // could. This affects performance but not correctness.
+       //
+       // Longer term, we could fix the forgot-to-scale bug (and look carefully
+       // for correctness regressions; https://codereview.appspot.com/5494068
+       // landed in 2011), or replace this atof algorithm with a faster one (e.g.
+       // Ryu). Shorter term, this comment will suffice.
        const errorscale = 8
-       errors := 0 // An upper bound for error, computed in errorscale*ulp.
+
+       errors := 0 // An upper bound for error, computed in ULP/errorscale.
        if trunc {
                // the decimal number was truncated.
                errors += errorscale / 2