code review 178140043: math/big: first cut at multi-precision Floats

math/big: first cut at multi-precision Floats

- implemented all rounding modes
- implemented +, -, *, /, and some unary ops
- implemented basic conversions
- written basic documentation

With precision set to 24 or 53, the results match
float32 or float64 operations exactly.

Known bugs: / doesn't round correctly in some cases.

CL IN PROGRESS.

[gri, 2014-11-22 01:26:03]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[gri, 2014-11-22 01:28:24]

Sending this out for interested parties. No need for thorough review yet -
there's lots of stuff missing. But it is starting to come together.

API feedback welcome.

On Fri Nov 21 2014 at 5:26:04 PM <gri@golang.org> wrote:

> Reviewers: adonovan,
>
> Message:
> Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> math/big: first cut at multi-precision Floats
>
> Working:
> - rounding to nearest
> - addition, subtraction (mostly), multiplication
> - basic conversions
>
> With precision set to 24 or 53, the results match
> float32 or float64 operations exactly.
>
> CL IN PROGRESS.
>
> Please review this at https://codereview.appspot.com/178140043/
>
> Affected files (+1189, -5 lines):
>    A src/math/big/float.go
>    A src/math/big/float_test.go
>    M src/math/big/nat.go
>    M src/math/big/nat_test.go
>
>
>

[adonovan, 2014-11-22 22:09:32]

API looks good, although you have many docstrings still to add.

What's the motivation here?  Use in the now-in-Go gc frontend, obviously, but
that only needs prec=256.   What else?

It would be interesting to record and replay the API calls done by gc compiling
GOPATH+GOTOOL and see how the exact.Value implementation compares in time and
space.

I'm going to need to block out a couple of days to review this, when you're
ready.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:15: // - All operations that round should return a 2nd
result indicating accuracy of operation
That would make the API hard to use, and the z.Add(x,y,mode) is already too
hard.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:25: // A Float represents a multi-precision floating point
number.
Some paragraph breaks would help.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:27: // Each value has its own precision > 0 which is the
number of
">= 0"  (according to Prec)

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:33: // precision given by z. By setting the desired
precision to 24
Mention how one sets the desired precision (e.g. call to Round).

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:36: // iEEE-754 operations (except for denormalized
numbers and NaNs).
"IEEE"

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:48: // a result there may be additional trailing zero's).
The value is
"zeros"

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:49: // sign * 0.mantissa * 2**exp (similar to IEEE-754
floating point).
not 1.mantissa?

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:181: func (z *Float) Round(x *Float, prec uint, mode
RoundingMode) *Float {
docstring

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:205: // Precision is set to 64bits.
"64 bits"

(everywhere)

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:269: // Precision is set to the bit size of x.
define bit size?

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:315: func (x *Float) Uint64() uint64 {
docstring

(everywhere)

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:632: // Cmp compares x and y and returns:
// ... returns the signum of x-y.

then delete the rest

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:668: // Sign returns:
// Sign returns the signum of x.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:713: func (z *Float) SetString(s string) (*Float, bool) {
Finish the docstring.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:717: func (x *Float) String() string {
specify

[adonovan, 2014-11-22 22:10:13]

On 2014/11/22 22:09:32, adonovan wrote:
> It would be interesting to record and replay the API calls done by gc
compiling
> GOPATH+GOTOOL and see how the exact.Value implementation compares in time and

Er, where GOTOOL=golang.org/x/tools/...

[gri, 2014-12-01 19:25:13]

FYI.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:15: // - All operations that round should return a 2nd
result indicating accuracy of operation
On 2014/11/22 22:09:31, adonovan wrote:
> That would make the API hard to use, and the z.Add(x,y,mode) is already too
> hard.

I will probably change so that each operation just returns whether the result is
precise or not. Chaining doesn't work well either way. The precision result can
be used to implement interval-arithmetic.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:25: // A Float represents a multi-precision floating point
number.
On 2014/11/22 22:09:31, adonovan wrote:
> Some paragraph breaks would help.

Will fix eventually.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:27: // Each value has its own precision > 0 which is the
number of
On 2014/11/22 22:09:31, adonovan wrote:
> ">= 0"  (according to Prec)

yes, but TBD

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:33: // precision given by z. By setting the desired
precision to 24
On 2014/11/22 22:09:31, adonovan wrote:
> Mention how one sets the desired precision (e.g. call to Round).

Will do eventually when API is clearer.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:36: // iEEE-754 operations (except for denormalized
numbers and NaNs).
On 2014/11/22 22:09:31, adonovan wrote:
> "IEEE"

Done.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:48: // a result there may be additional trailing zero's).
The value is
On 2014/11/22 22:09:31, adonovan wrote:
> "zeros"

Done.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:49: // sign * 0.mantissa * 2**exp (similar to IEEE-754
floating point).
On 2014/11/22 22:09:31, adonovan wrote:
> not 1.mantissa?

No. The implicit 1 make sense for hardware and with a small number of bits
available. It would only the software implementation unduly.

Basically, the mantissa value m is guaranteed to be 0.5 <= m < 1; or 0 (in case
of 0).

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:181: func (z *Float) Round(x *Float, prec uint, mode
RoundingMode) *Float {
On 2014/11/22 22:09:31, adonovan wrote:
> docstring

Acknowledged.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:205: // Precision is set to 64bits.
On 2014/11/22 22:09:31, adonovan wrote:
> "64 bits"
> 
> (everywhere)

Done.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:269: // Precision is set to the bit size of x.
On 2014/11/22 22:09:31, adonovan wrote:
> define bit size?

Done.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:315: func (x *Float) Uint64() uint64 {
On 2014/11/22 22:09:31, adonovan wrote:
> docstring
> 
> (everywhere)

Acknowledged.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:632: // Cmp compares x and y and returns:
On 2014/11/22 22:09:31, adonovan wrote:
> // ... returns the signum of x-y.
> 
> then delete the rest

There may be issues with -0 and +0 (TBD). Also, this is the way Cmp is
documented for Ints and Rats. It's more direct. Will leave as is.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:668: // Sign returns:
On 2014/11/22 22:09:31, adonovan wrote:
> // Sign returns the signum of x.

This is the way it's documented for Int's and Rat's. Will leave as is. It's a
direct definition.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:713: func (z *Float) SetString(s string) (*Float, bool) {
On 2014/11/22 22:09:31, adonovan wrote:
> Finish the docstring.

Acknowledged.

https://codereview.appspot.com/178140043/diff/60001/src/math/big/float.go#new...
src/math/big/float.go:717: func (x *Float) String() string {
On 2014/11/22 22:09:31, adonovan wrote:
> specify

Acknowledged.

[gri, 2014-12-03 00:09:17]

Hello adonovan@google.com (cc: golang-codereviews@googlegroups.com),

Please take another look.

[adonovan, 2014-12-03 22:15:03]

Not just an API review, but I skipped some of the gnarlier methods for now.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:38: // word x.mant[0]. The zero Float value is represented
by an
State that there are many representations of zero:
- len(mant)==0, nil or non-nil
- any sign
- any value of exp

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:51: // result to the precision of the result argument
according to m.
"rounding mode round ... according to the specified mode" (no m)

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:51: // result to the precision of the result argument
according to m.
I think you need to call out more explicitly that the receiver object influences
the result, which is not usual in the math/big API; more often it's treated as a
variable to clobber.

An example (or example_test.go) would help.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:53: // rounding, Float operations can emulate float32 (or
float64) IEEE-754
remove parens

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:71: //  -1: float value is below exact value
Don't overspecify; say <0, 0, >0.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:105: ToNegInf     = infMode | ToZero       // round to
-infinity (i.e., always round down)
"| ToZero" and "| AwayFromZero" are misleading here.
Why not just |1, |2?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:125: // Prec returns the mantissa precision of x in bits.
Precision

It's unlikely to be commonly used.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:126: // The precision may be 0 if x == 0. // TODO(gri) FIX
THIS
Why is it wrong?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:131: // Acc returns the accuracy of x returned from an
operation.
Accuracy

ditto

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:143: if x.prec <= 0 {
prec is unsigned so <0 is impossible.

This function seems to have a precondition that x is nonzero.
Document or fix that.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:287: func (z *Float) Round(x *Float, prec uint, mode
RoundingMode) *Float {
docstring

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:294: // nlz returns the number of leading zero bits in x.
Move out into a util.go file?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:325: func (z *Float) SetInt64(x int64) *Float {
Make sure you test this for MinInt64.  The code LG though.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:337: func (z *Float) SetFloat64(x float64) *Float {
Test with denormals and NaN, or document precondition.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:345: z.neg = x < 0
Move this before the x==0 check and eliminate L340?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:379: // z.prec = ?
0?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:418: func (x *Float) Uint64() uint64 {
docstring

Which rounding mode?
What about numbers outside [0..2^64)?
Return exactness/accuracy?

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:430: return -v
This looks wrong.
v will be zero if x is negative.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:435: func (x *Float) Float64() (float64, Accuracy) {
docstring

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:441: s := uint64(0)
var sign uint64

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:443: s = 1
sign = 1<<63

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:452: return nil
This must return non-nil.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:465: if x.exp < 0 {
x.exp <= 0 

I don't think len(x.mant) > 0 => x.exp > 0 is a representation invariant.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:482: // TODO(gri) do we return -0 for x == 0?
Yes, we should.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:634: func (z *Float) Add(x, y *Float, mode RoundingMode)
*Float {
(See my comment about the magical influence of z.prec on the result.)

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:635: // TODO(gri) what about -0?
IEEE 754 specifies all the algebraic identities for addition of -0.
I forget what they are, but we should follow them.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:666: // TODO(gri) what about -0?
ditto

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:701: z.neg = false
x.neg XOR y.neg

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:754: // x cmp (-y) == x
This comment is very cryptic.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:768: return
I wouldn't bother with r, just say return in each case.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:810: // number    = [ sign ] mantissa [ exponent ] .
docstring

[gri, 2014-12-03 23:42:07]

thanks

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:38: // word x.mant[0]. The zero Float value is represented
by an
On 2014/12/03 22:15:02, adonovan wrote:
> State that there are many representations of zero:
> - len(mant)==0, nil or non-nil
> - any sign
> - any value of exp

This may change. Added TODO.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:51: // result to the precision of the result argument
according to m.
On 2014/12/03 22:15:02, adonovan wrote:
> "rounding mode round ... according to the specified mode" (no m)

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:51: // result to the precision of the result argument
according to m.
On 2014/12/03 22:15:02, adonovan wrote:
> I think you need to call out more explicitly that the receiver object
influences
> the result, which is not usual in the math/big API; more often it's treated as
a
> variable to clobber.
> 
> An example (or example_test.go) would help.

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:53: // rounding, Float operations can emulate float32 (or
float64) IEEE-754
On 2014/12/03 22:15:03, adonovan wrote:
> remove parens

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:71: //  -1: float value is below exact value
On 2014/12/03 22:15:03, adonovan wrote:
> Don't overspecify; say <0, 0, >0.

I agree 100% but that's how it's done for Ints and Rats. And still agree 100%
but I got pushback from others when I did it as you suggested - the rationale
being predictability. If it's not specified, people will still find out it's -1,
0, 1 and then rely on it.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:105: ToNegInf     = infMode | ToZero       // round to
-infinity (i.e., always round down)
On 2014/12/03 22:15:03, adonovan wrote:
> "| ToZero" and "| AwayFromZero" are misleading here.
> Why not just |1, |2?

ToNegInf is ToZero for x > 0 . Also, if I use 1 then I need to repeat the
RoundingMode type. The actual values are unimportant for clients.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:125: // Prec returns the mantissa precision of x in bits.
On 2014/12/03 22:15:03, adonovan wrote:
> Precision
> 
> It's unlikely to be commonly used.

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:126: // The precision may be 0 if x == 0. // TODO(gri) FIX
THIS
On 2014/12/03 22:15:02, adonovan wrote:
> Why is it wrong?

0 precision is usually not allowed. I need to determine what to do for the 0
case.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:131: // Acc returns the accuracy of x returned from an
operation.
On 2014/12/03 22:15:03, adonovan wrote:
> Accuracy
> 
> ditto

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:143: if x.prec <= 0 {
On 2014/12/03 22:15:02, adonovan wrote:
> prec is unsigned so <0 is impossible.
> 
> This function seems to have a precondition that x is nonzero.
> Document or fix that.

I know < 0 is impossible. But I wrote <= 0 so that one can trivially conclude
it's > 0 if we pass.

Anyway, changed and documented.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:287: func (z *Float) Round(x *Float, prec uint, mode
RoundingMode) *Float {
On 2014/12/03 22:15:01, adonovan wrote:
> docstring

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:294: // nlz returns the number of leading zero bits in x.
On 2014/12/03 22:15:02, adonovan wrote:
> Move out into a util.go file?

eventually

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:325: func (z *Float) SetInt64(x int64) *Float {
On 2014/12/03 22:15:02, adonovan wrote:
> Make sure you test this for MinInt64.  The code LG though.

already done

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:337: func (z *Float) SetFloat64(x float64) *Float {
On 2014/12/03 22:15:02, adonovan wrote:
> Test with denormals and NaN, or document precondition.

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:345: z.neg = x < 0
On 2014/12/03 22:15:02, adonovan wrote:
> Move this before the x==0 check and eliminate L340?

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:379: // z.prec = ?
On 2014/12/03 22:15:03, adonovan wrote:
> 0?

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:418: func (x *Float) Uint64() uint64 {
On 2014/12/03 22:15:02, adonovan wrote:
> docstring
> 
> Which rounding mode?
> What about numbers outside [0..2^64)?
> Return exactness/accuracy?

added TODO

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:430: return -v
On 2014/12/03 22:15:03, adonovan wrote:
> This looks wrong.
> v will be zero if x is negative.

?

it's tested for positive and negative numbers

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:435: func (x *Float) Float64() (float64, Accuracy) {
On 2014/12/03 22:15:02, adonovan wrote:
> docstring

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:441: s := uint64(0)
On 2014/12/03 22:15:02, adonovan wrote:
> var sign uint64

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:443: s = 1
On 2014/12/03 22:15:03, adonovan wrote:
> sign = 1<<63

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:452: return nil
On 2014/12/03 22:15:02, adonovan wrote:
> This must return non-nil.

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:465: if x.exp < 0 {
On 2014/12/03 22:15:02, adonovan wrote:
> x.exp <= 0 
> 
> I don't think len(x.mant) > 0 => x.exp > 0 is a representation invariant.

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:482: // TODO(gri) do we return -0 for x == 0?
On 2014/12/03 22:15:02, adonovan wrote:
> Yes, we should.

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:634: func (z *Float) Add(x, y *Float, mode RoundingMode)
*Float {
On 2014/12/03 22:15:02, adonovan wrote:
> (See my comment about the magical influence of z.prec on the result.)

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:635: // TODO(gri) what about -0?
On 2014/12/03 22:15:02, adonovan wrote:
> IEEE 754 specifies all the algebraic identities for addition of -0.
> I forget what they are, but we should follow them.

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:666: // TODO(gri) what about -0?
On 2014/12/03 22:15:02, adonovan wrote:
> ditto

Acknowledged.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:701: z.neg = false
On 2014/12/03 22:15:03, adonovan wrote:
> x.neg XOR y.neg

hence the TODO

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:754: // x cmp (-y) == x
On 2014/12/03 22:15:02, adonovan wrote:
> This comment is very cryptic.

same style as in Add, Sub, Mul

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:768: return
On 2014/12/03 22:15:02, adonovan wrote:
> I wouldn't bother with r, just say return in each case.

Done.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:810: // number    = [ sign ] mantissa [ exponent ] .
On 2014/12/03 22:15:03, adonovan wrote:
> docstring

This is incomplete

[adonovan, 2014-12-03 23:49:11]

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:430: return -v
On 2014/12/03 23:42:05, gri wrote:
> On 2014/12/03 22:15:03, adonovan wrote:
> > This looks wrong.
> > v will be zero if x is negative.
> 
> ?
> 
> it's tested for positive and negative numbers

Ah.  That's because Uint64() returns the absolute value, which seems like a bug.
 -1 should not silently become 1.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:754: // x cmp (-y) == x
On 2014/12/03 23:42:06, gri wrote:
> On 2014/12/03 22:15:02, adonovan wrote:
> > This comment is very cryptic.
> 
> same style as in Add, Sub, Mul

The other ones make notational sense, but these ones don't.
In particular, the middle two cases are not real equations.
I think you mean something like:
   |x| cmp (-|y|) = 1

[gri, 2014-12-04 00:06:36]

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go
File src/math/big/float.go (right):

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:430: return -v
On 2014/12/03 23:49:11, adonovan wrote:
> On 2014/12/03 23:42:05, gri wrote:
> > On 2014/12/03 22:15:03, adonovan wrote:
> > > This looks wrong.
> > > v will be zero if x is negative.
> > 
> > ?
> > 
> > it's tested for positive and negative numbers
> 
> Ah.  That's because Uint64() returns the absolute value, which seems like a
bug.
>  -1 should not silently become 1.

Added TODO. The current implementation borrows from the Int.Int64 implementation
and semantics.

https://codereview.appspot.com/178140043/diff/190005/src/math/big/float.go#ne...
src/math/big/float.go:754: // x cmp (-y) == x
On 2014/12/03 23:49:11, adonovan wrote:
> On 2014/12/03 23:42:06, gri wrote:
> > On 2014/12/03 22:15:02, adonovan wrote:
> > > This comment is very cryptic.
> > 
> > same style as in Add, Sub, Mul
> 
> The other ones make notational sense, but these ones don't.
> In particular, the middle two cases are not real equations.
> I think you mean something like:
>    |x| cmp (-|y|) = 1

Done.

[gobot, 2014-12-19 05:16:29]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/178140043 is best) in the description in your
new CL.

Thanks very much.