Add NaN and Inf support; allow result-as-operand

[maddyblue]

This is a fairly big PR and incorporates two different features. I can break it up into 2 PRs if you prefer, but it's at least different commits. No rush on this since it's pretty big. Also I'm happy to find another reviewer if you'd prefer to not do it.

---

This change is 

[eisenstatdavid]

I started reviewing today. No need to split, but yeah, this review will take a while.

[eisenstatdavid]

This PR is in good shape despite the many comments below.

On subject of testing, I haven't looked at the IBM tests carefully (and the exclusions) because I know that they're pretty comprehensive, but do you know what kind of branch coverage they achieve on this code?

---

Reviewed 8 of 8 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

const.go, line 79 at r1 (raw file):

			MaxExponent: MaxExponent,
			MinExponent: MinExponent,
		}

Could there be other places that need this fix?

---

context.go, line 125 at r1 (raw file):

	}
	xn := x.Negative
	yn := y.Negative

Maybe unify the branches below with yn := y.Negative != subtract? The final result would be sth like

xn := x.Negative
yn := y.Negative != subtract  // XOR
if xi, yi := x.Form == Infinite, y.Form == Infinite; xi || yi {
  if xi && yi && xn != yn {
    d.Set(decimalNaN)
    return c.goError(InvalidOperation)
  } else if xi {
    d.Set(x)
  } else {
    d.Set(decimalInfinity)
    d.Negative = yn
  }
  return 0, nil
}

---

context.go, line 173 at r1 (raw file):

	if subtract {
		yn = !yn
	}

This would be subsumed by yn := y.Negative != subtract above.

---

context.go, line 175 at r1 (raw file):

	}
	subtract = xn != yn
	if subtract {

Since subtract is never used again, perhaps change the test to xn == yn (and reverse the branches).

For this branch, atomically with the below suggestion, suggest

if xn {
  d.Coeff.Sub(b, a)
} else {
  d.Coeff.Sub(a, b)
}
// special case for RoundFloor on the following line if necessary
d.Negative = d.Coeff.Sign() < 0
d.Coeff.Abs(&d.Coeff)

---

context.go, line 179 at r1 (raw file):

		d.Coeff.Abs(&d.Coeff)
	} else {
		d.Coeff.Add(a, b)

Atomically with the above suggestion, suggest d.Negative = xn here. This logic covers negative zero + negative zero and negative zero - positive zero.

---

context.go, line 189 at r1 (raw file):

		}
	} else {
		d.Negative = xn && yn

&& x.Coeff.Sign() == 0 && y.Coeff.Sign() == 0 unless the spec has a gratuitous difference from IEEE (but see above). Is this case tested?

---

context.go, line 232 at r1 (raw file):

		return res, err
	}
	if xi, yi := x.Form == Infinite, y.Form == Infinite; xi || yi {

This logic could be simplified to

if x.Form == Infinite || y.Form == Infinite {
  if x.IsZero() || y.IsZero() {
    d.Set(decimalNaN)
    res = InvalidOperation
  } else {
    d.Set(decimalInfinity)
    d.Negative = x.Negative != y.Negative
  }
  return c.goError(res)
}

Definitely at least suppress the special case for xi && yi.

---

context.go, line 280 at r1 (raw file):

		} else if xi {
			d.Set(decimalInfinity)
			d.Negative = x.Negative != y.Negative

Don't we need to check whether y is zero and set a flag if so? Should all of this added code come before the precision check?

---

context.go, line 283 at r1 (raw file):

		} else {
			d.SetInt64(0)
			d.Exponent = c.etiny()

Idle question: how does setting a very small exponent interact with upscale?

---

context.go, line 410 at r1 (raw file):

	}
	var res Condition
	if xi, yi := x.Form == Infinite, y.Form == Infinite; xi || yi {

Could the duplicate logic here be pulled out as (e.g.) quoSpecials? Maybe there's another way to reduce the code duplication across division routines.

---

context.go, line 441 at r1 (raw file):

	d.Form = Finite
	if d.NumDigits() > int64(c.Precision) {
		d.Set(decimalNaN)

Why NaN instead of rounding? The spec seems silent on this issue.

---

context.go, line 462 at r1 (raw file):

	}
	if y.Form == Infinite {
		d.Set(x)

Don't we need to invert the sign when y is negative? IIRC, floating point division should satisfy -x / y = x / -y = -(x / y).

---

context.go, line 617 at r1 (raw file):

	//     x_{n+1} = 1/3 * ( 2 * x_n + (d / x_n / x_n) ).

	if set, res, err := c.rootSpecials(d, x); set {

Does this Cbrt return NaN on negative inputs? Should it?

---

context.go, line 891 at r1 (raw file):

		return res, err
	}
	if x.Sign() < 0 {

logSpecials?

---

context.go, line 934 at r1 (raw file):

	// Transactions on Mathematical Software, Vol 12 #2, pp79-91, ACM, June 1986.

	if set, res, err := c.setIfNaN(d, x); set {

How many of these functions need to set the Form field? I guess the transcendental functions don't because they're computed by iterative algorithms that use lower-level functions.

---

context.go, line 1214 at r1 (raw file):

func (c *Context) quantize(d, v *Decimal, exp int32) Condition {
	if v.Form == Infinite || exp < c.etiny() {

Could this check go in Quantize? I'm not sure why we would test d.NumDigits() when d is not a number.

---

decimal.go, line 43 at r1 (raw file):

const (
	// These are in total-ordering order, and so cannot be otherwise sorted.

I had to read this comment a couple times to get it. Perhaps "CmpTotal assumes that the order of these constants reflects the total order on decimals"? cmpOrder seems to make further assumptions, which may be worth mentioning.

---

decimal.go, line 114 at r1 (raw file):

		isNaN = true
		d.Form = NaN
		s = s[3:]

Rather than count here, perhaps introduce a function like

func ConsumePrefix(s, prefix string) (string, bool) {
  if strings.HasPrefix(s, prefix) {
    return s[len(prefix):], true
  }
  return s, false
}

and use throughout.

---

decimal.go, line 132 at r1 (raw file):

	}

	// Until there are no parse errors, leave as NaN.

Couldn't this be done earlier?

---

decimal.go, line 136 at r1 (raw file):

	var exps []int64
	if i := strings.IndexAny(s, "eE"); i >= 0 {

We're only looking for lowercase e now, so we can use IndexByte.

---

decimal.go, line 244 at r1 (raw file):

	var s string

	switch d.Form {

Is it worth pulling out the duplicate handling for specials and negative numbers between this function and the previous function?

---

decimal.go, line 511 at r1 (raw file):

	switch d.Form {
	case Finite:
		// d and x have the same sign and form, compare their value.

Unless I'm mistaken, cmpOrder returns 0 for all finite decimals, both negative and positive, so d and x don't necessarily have the same sign. I guess this works anyway because Cmp handles the signs too. See below.

---

decimal.go, line 536 at r1 (raw file):

	v := int(d.Form)
	if d.Negative {
		v = -v

Probably v = ^v (or v = -(v + 1) if that's too clever); see above.

---

decimal.go, line 568 at r1 (raw file):

	ds := d.Sign()
	xs := x.Sign()
	gt := 1

Wait to define these until after checking the signs and leave a comment explaining them. It looks strange at first that the sign tests use ±1 while the rest uses the "constants".

---

decimal.go, line 620 at r1 (raw file):

	}

	funcs := []func() bool{

I'm confused by the inner functions. We never seem to set v and then not return true, so couldn't we just return from the outer function instead?

---

decimal.go, line 666 at r1 (raw file):

//	+1 if d.Negative == false
//
func (d *Decimal) Sign() int {

I prefer

if d.Form == Finite && d.Coeff.Sign() == 0 {
  return 0
}
if d.Negative {
  return -1
}
return 1

---

decimal.go, line 686 at r1 (raw file):

// IsZero returns true if d == 0 or -0.
func (d *Decimal) IsZero() bool {
	return d.Sign() == 0 && d.Form == Finite

Is the Form check necessary?

---

gda_test.go, line 449 at r1 (raw file):

					t.Skip("x ** large y")
				}
			case "quantize":

Doesn't Quantize handle infinite operands now?

---

gda_test.go, line 627 at r1 (raw file):

					tc.Result = "sNaN"
				}
				if strings.ToUpper(s) != strings.ToUpper(tc.Result) {

strings.EqualFold? Kind of pedantic here admittedly.

---

Comments from Reviewable

[maddyblue]

I ran go test -coverprofile which had a 93.6% test coverage. Most of the non-coverage was for error handling lines that were never triggered and some string and other misc stuff that I'll add soon. But all of the meat in decimal.go and context.go appears to be covered. The only thing not covered are two branches in Sqrt that attempt to adjust for off-by-one errors. These will be investigated.

---

Review status: 6 of 8 files reviewed at latest revision, 29 unresolved discussions.

---

const.go, line 79 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Could there be other places that need this fix?

I can't think of any.

---

context.go, line 125 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Maybe unify the branches below with yn := y.Negative != subtract? The final result would be sth like

xn := x.Negative
yn := y.Negative != subtract  // XOR
if xi, yi := x.Form == Infinite, y.Form == Infinite; xi || yi {
  if xi && yi && xn != yn {
    d.Set(decimalNaN)
    return c.goError(InvalidOperation)
  } else if xi {
    d.Set(x)
  } else {
    d.Set(decimalInfinity)
    d.Negative = yn
  }
  return 0, nil
}

I spent a few minutes trying to get this to work (that is, all of your comments about this function) but failed, as some small subset of tests always failed. I agree it seems like there's a simpler solution than what we have here but I currently think it should be done in another PR. I've created #37 to track this.

---

context.go, line 175 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Since subtract is never used again, perhaps change the test to xn == yn (and reverse the branches).

For this branch, atomically with the below suggestion, suggest

if xn {
  d.Coeff.Sub(b, a)
} else {
  d.Coeff.Sub(a, b)
}
// special case for RoundFloor on the following line if necessary
d.Negative = d.Coeff.Sign() < 0
d.Coeff.Abs(&d.Coeff)

I've made the xn == yn change, but not larger one.

---

context.go, line 189 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

&& x.Coeff.Sign() == 0 && y.Coeff.Sign() == 0 unless the spec has a gratuitous difference from IEEE (but see above). Is this case tested?

Yes, this branch is covered during testing.

---

context.go, line 232 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

This logic could be simplified to

if x.Form == Infinite || y.Form == Infinite {
  if x.IsZero() || y.IsZero() {
    d.Set(decimalNaN)
    res = InvalidOperation
  } else {
    d.Set(decimalInfinity)
    d.Negative = x.Negative != y.Negative
  }
  return c.goError(res)
}

Definitely at least suppress the special case for xi && yi.

This change fails 18 test cases.

---

context.go, line 280 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Don't we need to check whether y is zero and set a flag if so? Should all of this added code come before the precision check?

I don't think we need a y == 0 check here because it's covered in the tests. For example (

apd/testdata/divide.decTest

Line 597 in c7ade12

divx784 divide Inf 0 -> Infinity

):

divx784 divide  Inf   0     ->  Infinity

(This is Inf / 0 == Inf)

The precision check was added by me to prevent footgun problems. I think that this block should stay above the precision check because in these cases we can produce correct, fast results.

---

context.go, line 283 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Idle question: how does setting a very small exponent interact with upscale?

upscale returns an error if the differing exponents are too different:

apd/decimal.go

Line 350 in c7ade12

s := int64(a.Exponent) - int64(b.Exponent)

---

context.go, line 410 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Could the duplicate logic here be pulled out as (e.g.) quoSpecials? Maybe there's another way to reduce the code duplication across division routines.

Done.

---

context.go, line 441 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Why NaN instead of rounding? The spec seems silent on this issue.

The spec says: "The exponent of the result must be 0. Hence, if the result cannot be expressed exactly within precision digits, the operation is in error and will fail – that is, the result cannot have more digits than the value of precision in effect for the operation, and will not be rounded."

http://speleotrove.com/decimal/daops.html#refdivint

---

context.go, line 462 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Don't we need to invert the sign when y is negative? IIRC, floating point division should satisfy -x / y = x / -y = -(x / y).

The spec says: "The sign of the result, if non-zero, is the same as that of the original dividend."

http://speleotrove.com/decimal/daops.html#refremain

So I guess we don't satisfy that.

---

context.go, line 617 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Does this Cbrt return NaN on negative inputs? Should it?

It does (see the changes to cuberoot-apd.decTest), and it should do exactly the same thing Sqrt does for these inputs, I think.

---

context.go, line 891 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

logSpecials?

Done.

---

context.go, line 934 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

How many of these functions need to set the Form field? I guess the transcendental functions don't because they're computed by iterative algorithms that use lower-level functions.

All functions must set the Form field. I added stuff to gda_test.go that fills in a bogus Form value to make sure it always gets set. And yes, the transcendental functions don't need to explicitly set it because of the simpler functions that do. But overall I'm convinced that all branches set Form because of the gda_test modification described.

---

context.go, line 1214 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Could this check go in Quantize? I'm not sure why we would test d.NumDigits() when d is not a number.

Done.

---

decimal.go, line 43 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

I had to read this comment a couple times to get it. Perhaps "CmpTotal assumes that the order of these constants reflects the total order on decimals"? cmpOrder seems to make further assumptions, which may be worth mentioning.

Done. But I'm not sure what further assumptions you are referring to.

---

decimal.go, line 114 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Rather than count here, perhaps introduce a function like

func ConsumePrefix(s, prefix string) (string, bool) {
  if strings.HasPrefix(s, prefix) {
    return s[len(prefix):], true
  }
  return s, false
}

and use throughout.

Done.

---

decimal.go, line 132 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Couldn't this be done earlier?

Done.

---

decimal.go, line 136 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

We're only looking for lowercase e now, so we can use IndexByte.

Done.

---

decimal.go, line 244 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Is it worth pulling out the duplicate handling for specials and negative numbers between this function and the previous function?

Done.

---

decimal.go, line 511 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Unless I'm mistaken, cmpOrder returns 0 for all finite decimals, both negative and positive, so d and x don't necessarily have the same sign. I guess this works anyway because Cmp handles the signs too. See below.

Good catch. I didn't put together that Finite == -Finite since it's 0.

---

decimal.go, line 536 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Probably v = ^v (or v = -(v + 1) if that's too clever); see above.

This triggered a huge exploration that uncovered various bugs in CmpTotal and add, including the elusive RoundFloor negative thing that wasn't tested. Also another way to achieve this that I find a bit easier to understand, is doing v := int(d.Form) + 1 above. The +1 was more intuitive for me instead of the ^. Good find.

---

decimal.go, line 568 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Wait to define these until after checking the signs and leave a comment explaining them. It looks strange at first that the sign tests use ±1 while the rest uses the "constants".

Done.

---

decimal.go, line 620 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

I'm confused by the inner functions. We never seem to set v and then not return true, so couldn't we just return from the outer function instead?

I think I did this for a better reason that then got refactored away. Removed this functions.

---

decimal.go, line 666 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

I prefer

if d.Form == Finite && d.Coeff.Sign() == 0 {
  return 0
}
if d.Negative {
  return -1
}
return 1

Done.

---

decimal.go, line 686 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Is the Form check necessary?

I thought it was, but since Sign() can only return 0 if d is Finite, I agree it isn't necessary. Removed.

---

gda_test.go, line 449 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Doesn't Quantize handle infinite operands now?

This is checking the second operand. Quantize takes an int32 instead of a Decimal as the second operand, and so we skip all tests where the second operand isn't finite.

---

gda_test.go, line 627 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

strings.EqualFold? Kind of pedantic here admittedly.

Done.

---

Comments from Reviewable

[eisenstatdavid]

Thanks for writing additional tests.

---

Reviewed 1 of 8 files at r1, 1 of 1 files at r4, 7 of 7 files at r5.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

context.go, line 125 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I spent a few minutes trying to get this to work (that is, all of your comments about this function) but failed, as some small subset of tests always failed. I agree it seems like there's a simpler solution than what we have here but I currently think it should be done in another PR. I've created #37 to track this.

OK, I'll try again out of band.

---

context.go, line 280 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

I don't think we need a y == 0 check here because it's covered in the tests. For example (

apd/testdata/divide.decTest

Line 597 in c7ade12

divx784 divide Inf 0 -> Infinity

):

divx784 divide  Inf   0     ->  Infinity

(This is Inf / 0 == Inf)

The precision check was added by me to prevent footgun problems. I think that this block should stay above the precision check because in these cases we can produce correct, fast results.

I guess this is a special case of positive decimal divided by zero.

---

context.go, line 617 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

It does (see the changes to cuberoot-apd.decTest), and it should do exactly the same thing Sqrt does for these inputs, I think.

Weird - - I would have expected it to copy the sign of its input, since x^3 - a has exactly one real root for all a. I don't remember if SQL has cbrt, but if it does, we should think about the alignment between this function and IEEE cbrt, which accepts negative numbers.

---

context.go, line 1257 at r5 (raw file):

	}
	d.Exponent = exp
	d.Negative = neg

Do we mutate either v or neg above? Wondering why this isn't d.Negative = v.Negative or suppressed entirely by dint of the d.Set(v) above.

I guess I can answer my own question: because d and v may be aliased.

---

decimal.go, line 43 at r1 (raw file):

Previously, mjibson (Matt Jibson) wrote…

Done. But I'm not sure what further assumptions you are referring to.

That for all constant values c1 and c2, -(c1 + 1) is less than c2 + 1. Maybe not comment-worthy since I don't expect this enumeration to change, but a little unusual.

---

Comments from Reviewable

[maddyblue]

Review status: all files reviewed at latest revision, 11 unresolved discussions.

---

context.go, line 617 at r1 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Weird - - I would have expected it to copy the sign of its input, since x^3 - a has exactly one real root for all a. I don't remember if SQL has cbrt, but if it does, we should think about the alignment between this function and IEEE cbrt, which accepts negative numbers.

You are correct. Cbrt should support negative numbers. I didn't think that through completely. I've opened #38 to track this.

---

context.go, line 1257 at r5 (raw file):

Previously, eisenstatdavid (David Eisenstat) wrote…

Do we mutate either v or neg above? Wondering why this isn't d.Negative = v.Negative or suppressed entirely by dint of the d.Set(v) above.

I guess I can answer my own question: because d and v may be aliased.

I was able to remove this completely. The problem was the d.SetCoefficient(0) in the !d.IsZero block. Using Coeff.SetInt64 instead doesn't squash the negative setting.

---

Comments from Reviewable