3rd review comments

context.go

@@ -311,7 +311,7 @@ func (c *Context) Sqrt(d, x *Decimal) (Condition, error) {
 	f.Exponent = int32(-nd)
 	approx := new(Decimal)
 	nc := c.WithPrecision(c.Precision)
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 	if e%2 == 0 {
 		approx.SetCoefficient(819).SetExponent(-3)
 		ed.Mul(approx, approx, f)
@@ -328,7 +328,7 @@ func (c *Context) Sqrt(d, x *Decimal) (Condition, error) {
 	tmp := new(Decimal)
 	// The algorithm in the paper says to use c.Precision + 2. 7 instead of 2
 	// here allows all of the non-extended tests to pass without allowing 1ulp
-	// of error or ignoring the Inexact flag, similary to the Quo precision
+	// of error or ignoring the Inexact flag, similarly to the Quo precision
 	// increase. This does mean that there are probably some inputs for which
 	// Sqrt is 1ulp off or will incorrectly mark things as Inexact or exact.
 	for maxp := c.Precision + 7; p != maxp; {
@@ -344,9 +344,8 @@ func (c *Context) Sqrt(d, x *Decimal) (Condition, error) {
 		// approx = 0.5 * (approx + f / approx)
 		ed.Mul(approx, tmp, decimalHalf)
 	}
-	p = c.Precision
-	nc.Precision = p
-	dp := int32(p)
+	nc.Precision = c.Precision
+	dp := int32(c.Precision)
 	approxsubhalf := new(Decimal)
 	ed.Sub(approxsubhalf, approx, New(5, -1-dp))
 	nc.Rounding = RoundUp
@@ -394,7 +393,7 @@ func (c *Context) Cbrt(d, x *Decimal) (Condition, error) {
 
 	z := new(Decimal).Set(x)
 	nc := BaseContext.WithPrecision(c.Precision*2 + 2)
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 	exp8 := 0
 
 	// Follow Ken Turkowski paper:
@@ -492,7 +491,7 @@ func (c *Context) Ln(d, x *Decimal) (Condition, error) {
 
 	nc := c.WithPrecision(p)
 	nc.Rounding = RoundHalfEven
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 
 	tmp1 := new(Decimal)
 	tmp2 := new(Decimal)
@@ -676,7 +675,7 @@ func (c *Context) Exp(d, x *Decimal) (Condition, error) {
 
 	// Stage 4
 	nc.Precision = uint32(p)
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 	sum := New(1, 0)
 	tmp2.Exponent = 0
 	for i := n - 1; i > 0; i-- {
@@ -718,7 +717,7 @@ func (c *Context) integerPower(d, x *Decimal, y *big.Int) (Condition, error) {
 	n, z := new(Decimal), d
 	n.Set(x)
 	z.Set(decimalOne)
-	ed := NewErrDecimal(c)
+	ed := MakeErrDecimal(c)
 	for b.Sign() > 0 {
 		if b.Bit(0) == 1 {
 			ed.Mul(z, z, n)
@@ -804,7 +803,7 @@ func (c *Context) Pow(d, x, y *Decimal) (Condition, error) {
 	p += 4
 
 	nc := BaseContext.WithPrecision(p)
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 
 	ed.Abs(tmp, x)
 	ed.Ln(tmp, tmp)
@@ -923,5 +922,6 @@ func exp10(x int64) (f, exp *big.Int, err error) {
 		return nil, nil, errors.New(errExponentOutOfRangeStr)
 	}
 	f = big.NewInt(x)
+	// TODO(mjibson): use a table here.
 	return f, new(big.Int).Exp(bigTen, f, nil), nil
 }

decimal.go

@@ -34,6 +34,20 @@ type Decimal struct {
 	Exponent int32
 }
 
+const (
+	// TODO(mjibson): MaxExponent is set because both upscale and Round
+	// perform a calculation of 10^x, where x is an exponent. This is done by
+	// big.Int.Exp. This restriction could be lifted if better algorithms were
+	// determined during upscale and Round that don't need to perform Exp.
+
+	// MaxExponent is the highest exponent supported. Exponents near this range will
+	// perform very slowly (many seconds per operation).
+	MaxExponent = 100000
+	// MinExponent is the lowest exponent supported with the same limitations as
+	// MaxExponent.
+	MinExponent = -MaxExponent
+)
+
 // New creates a new decimal with the given coefficient and exponent.
 func New(coeff int64, exponent int32) *Decimal {
 	return &Decimal{
@@ -105,13 +119,17 @@ func (d *Decimal) String() string {
 
 // ToSci returns d in scientific notation if an exponent is needed.
 func (d *Decimal) ToSci() string {
+	// See: http://speleotrove.com/decimal/daconvs.html#reftostr
+	const adjExponentLimit = -6
+
 	s := d.Coeff.String()
-	neg := d.Coeff.Sign() < 0
-	if neg {
+	prefix := ""
+	if d.Coeff.Sign() < 0 {
+		prefix = "-"
 		s = s[1:]
 	}
 	adj := int(d.Exponent) + (len(s) - 1)
-	if d.Exponent <= 0 && adj >= -6 {
+	if d.Exponent <= 0 && adj >= adjExponentLimit {
 		if d.Exponent < 0 {
 			if left := -int(d.Exponent) - len(s); left > 0 {
 				s = "0." + strings.Repeat("0", left) + s
@@ -129,10 +147,7 @@ func (d *Decimal) ToSci() string {
 		}
 		s = fmt.Sprintf("%s%sE%+d", s[:1], dot, adj)
 	}
-	if neg {
-		s = "-" + s
-	}
-	return s
+	return prefix + s
 }
 
 // ToStandard converts d to a standard notation string (i.e., no exponent
@@ -301,23 +316,9 @@ func (d *Decimal) setExponent(c *Context, res Condition, xs ...int64) Condition
 	return res
 }
 
-const (
-	// TODO(mjibson): MaxExponent is set because both upscale and Round
-	// perform a calculation of 10^x, where x is an exponent. This is done by
-	// big.Int.Exp. This restriction could be lifted if better algorithms were
-	// determined during upscale and Round that don't need to perform Exp.
-
-	// MaxExponent is the highest exponent supported. Exponents near this range will
-	// perform very slowly (many seconds per operation).
-	MaxExponent = 100000
-	// MinExponent is the lowest exponent supported with the same limitations as
-	// MaxExponent.
-	MinExponent = -MaxExponent
-)
-
-// upscale converts a and b to big.Ints with the same scaling, and their
-// scaling. An error can be produced if the resulting scale factor is out
-// of range.
+// upscale converts a and b to big.Ints with the same scaling. It returns
+// them with this scaling, along with the scaling. An error can be produced
+// if the resulting scale factor is out of range.
 func upscale(a, b *Decimal) (*big.Int, *big.Int, int32, error) {
 	if a.Exponent == b.Exponent {
 		return &a.Coeff, &b.Coeff, a.Exponent, nil
@@ -333,14 +334,15 @@ func upscale(a, b *Decimal) (*big.Int, *big.Int, int32, error) {
 	if s > MaxExponent {
 		return nil, nil, 0, errors.New(errExponentOutOfRangeStr)
 	}
-	y := big.NewInt(s)
-	e := new(big.Int).Exp(bigTen, y, nil)
-	y.Mul(&a.Coeff, e)
-	x := &b.Coeff
+	x := big.NewInt(s)
+	// TODO(mjibson): use a table here.
+	e := new(big.Int).Exp(bigTen, x, nil)
+	x.Mul(&a.Coeff, e)
+	y := &b.Coeff
 	if swapped {
 		x, y = y, x
 	}
-	return y, x, b.Exponent, nil
+	return x, y, b.Exponent, nil
 }
 
 // Cmp compares d and x and returns:
@@ -391,6 +393,7 @@ func (d *Decimal) Cmp(x *Decimal) int {
 		diff = -diff
 	}
 	y := big.NewInt(diff)
+	// TODO(mjibson): use a table here.
 	e := new(big.Int).Exp(bigTen, y, nil)
 	db := new(big.Int).Set(&d.Coeff)
 	xb := new(big.Int).Set(&x.Coeff)
@@ -464,8 +467,8 @@ func (d *Decimal) Reduce(x *Decimal) *Decimal {
 	}
 	d.Set(x)
 
-	// Divide by 10 in a loop. In benchmarks this is 20% faster than converting
-	// to a string and trimming the 0s from the end.
+	// Divide by 10 in a loop. In benchmarks of reduce0.decTest, this is 20%
+	// faster than converting to a string and trimming the 0s from the end.
 	z := new(big.Int).Set(&d.Coeff)
 	r := new(big.Int)
 	for {

error.go

@@ -14,9 +14,9 @@
 
 package apd
 
-// NewErrDecimal creates a ErrDecimal with given context.
-func NewErrDecimal(c *Context) *ErrDecimal {
-	return &ErrDecimal{
+// MakeErrDecimal creates a ErrDecimal with given context.
+func MakeErrDecimal(c *Context) ErrDecimal {
+	return ErrDecimal{
 		Ctx: c,
 	}
 }

error_test.go

@@ -19,7 +19,7 @@ import "testing"
 // Appease the unused test.
 // TODO(mjibson): actually test all the ErrDecimal methods.
 func TestErrDecimal(t *testing.T) {
-	ed := NewErrDecimal(testCtx)
+	ed := MakeErrDecimal(testCtx)
 	a := New(1, 0)
 	ed.Abs(a, a)
 	ed.Exp(a, a)
@@ -32,15 +32,15 @@ func TestErrDecimal(t *testing.T) {
 	ed.Round(a, a)
 }
 
-// TestNewErrDecimalWithPrecision tests that WithPrecision generates a copy
+// TestMakeErrDecimalWithPrecision tests that WithPrecision generates a copy
 // and not a reference.
-func TestNewErrDecimalWithPrecision(t *testing.T) {
+func TestMakeErrDecimalWithPrecision(t *testing.T) {
 	c := &Context{
 		Precision:   5,
 		MaxExponent: 2,
 	}
 	nc := c.WithPrecision(c.Precision * 2)
-	ed := NewErrDecimal(nc)
+	ed := MakeErrDecimal(nc)
 	if ed.Ctx.Precision != 10 {
 		t.Fatalf("expected %d, got %d", 10, ed.Ctx.Precision)
 	}

gda_test.go

@@ -325,6 +325,9 @@ func (tc TestCase) Run(c *Context, done chan error, d, x, y *Decimal) (res Condi
 	return
 }
 
+// BenchmarkGDA benchmarks a GDA test. It should not be used without specifying
+// a sub-benchmark to run. For example:
+// go test -run XX -bench GDA/squareroot
 func BenchmarkGDA(b *testing.B) {
 	for _, fname := range GDAfiles {
 		b.Run(fname, func(b *testing.B) {