code review 1780042: exp/eval: Converted from bignum to big

exp/eval: Converted from bignum to big

Also in this CL:
* Removed util.go, as its functionality is in big
* Removed some semicolons from the code generated by gen.go
* Added a generate target to Makefile
* Removed an outdated TODO from value.go

[eds, 2010-07-10 01:03:43]

Hello gri (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[gri, 2010-07-12 19:36:45]

http://codereview.appspot.com/1780042/diff/2001/3003
File src/pkg/exp/eval/expr.go (right):

http://codereview.appspot.com/1780042/diff/2001/3003#newcode94
src/pkg/exp/eval/expr.go:94: var rat *big.Rat
if you make this

var rat big.Rat

you should be able to get rid of some of the new(big.Rat) calls below and just
use rat directly.

http://codereview.appspot.com/1780042/diff/2001/3003#newcode112
src/pkg/exp/eval/expr.go:112: rat = big.NewRat(0, 1).SetFrac(i, idealOne)
rat = new(big.Rat).SetInt(i)

or

rat.SetInt(i)

if you change the declaration of rat above

http://codereview.appspot.com/1780042/diff/2001/3003#newcode135
src/pkg/exp/eval/expr.go:135: // take absolute value
get rid of comment and use a helper function abs()

(big should probably have that one as well)

http://codereview.appspot.com/1780042/diff/2001/3003#newcode776
src/pkg/exp/eval/expr.go:776: f, ok := big.NewRat(0, 1).SetString(lit)
s/big.NewRat(0, 1)/new(big.Rat)/

http://codereview.appspot.com/1780042/diff/2001/3008
File src/pkg/exp/eval/type.go (right):

http://codereview.appspot.com/1780042/diff/2001/3008#newcode250
src/pkg/exp/eval/type.go:250: numer = numer.Add(numer, big.NewInt(-1))
Sub(numer, idealOne)

http://codereview.appspot.com/1780042/diff/2001/3008#newcode251
src/pkg/exp/eval/type.go:251: return big.NewRat(0, 1).SetFrac(numer, idealOne)
new(big.Rat).SetInt(numer)

http://codereview.appspot.com/1780042/diff/2001/3008#newcode320
src/pkg/exp/eval/type.go:320: return big.NewRat(0, 1).SetFrac(numer, idealOne)
new(big.Rat).SetInt(numer)

http://codereview.appspot.com/1780042/diff/2001/3008#newcode330
src/pkg/exp/eval/type.go:330: numer = numer.Add(numer, big.NewInt(-1))
Sub(numer, idealOne)

http://codereview.appspot.com/1780042/diff/2001/3008#newcode331
src/pkg/exp/eval/type.go:331: return big.NewRat(0, 1).SetFrac(numer, idealOne)
new(big.Rat).SetInt(numer)

http://codereview.appspot.com/1780042/diff/2001/3008#newcode1234
src/pkg/exp/eval/type.go:1234: maxFloat32Val = big.NewRat(0, 1).SetFrac(numer,
idealOne)
s/big.NewRat(0, 1)/new(big.Rat)/

apply also below

[gri, 2010-07-12 23:11:16]

Please see also the new functions added to pak/big this afternoon
(CL 1761042):

- Sign
- Abs
- IsInt

They should allow you to simplify the code in this CL quite a bit.
- gri

On Mon, Jul 12, 2010 at 12:36 PM, <gri@golang.org> wrote:

>
> http://codereview.appspot.com/1780042/diff/2001/3003
> File src/pkg/exp/eval/expr.go (right):
>
> http://codereview.appspot.com/1780042/diff/2001/3003#newcode94
> src/pkg/exp/eval/expr.go:94: var rat *big.Rat
> if you make this
>
> var rat big.Rat
>
> you should be able to get rid of some of the new(big.Rat) calls below
> and just use rat directly.
>
> http://codereview.appspot.com/1780042/diff/2001/3003#newcode112
> src/pkg/exp/eval/expr.go:112: rat = big.NewRat(0, 1).SetFrac(i,
> idealOne)
> rat = new(big.Rat).SetInt(i)
>
> or
>
> rat.SetInt(i)
>
> if you change the declaration of rat above
>
> http://codereview.appspot.com/1780042/diff/2001/3003#newcode135
> src/pkg/exp/eval/expr.go:135: // take absolute value
> get rid of comment and use a helper function abs()
>
> (big should probably have that one as well)
>
> http://codereview.appspot.com/1780042/diff/2001/3003#newcode776
> src/pkg/exp/eval/expr.go:776: f, ok := big.NewRat(0, 1).SetString(lit)
> s/big.NewRat(0, 1)/new(big.Rat)/
>
> http://codereview.appspot.com/1780042/diff/2001/3008
> File src/pkg/exp/eval/type.go (right):
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode250
> src/pkg/exp/eval/type.go:250: numer = numer.Add(numer, big.NewInt(-1))
> Sub(numer, idealOne)
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode251
> src/pkg/exp/eval/type.go:251: return big.NewRat(0, 1).SetFrac(numer,
> idealOne)
> new(big.Rat).SetInt(numer)
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode320
> src/pkg/exp/eval/type.go:320: return big.NewRat(0, 1).SetFrac(numer,
> idealOne)
> new(big.Rat).SetInt(numer)
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode330
> src/pkg/exp/eval/type.go:330: numer = numer.Add(numer, big.NewInt(-1))
> Sub(numer, idealOne)
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode331
> src/pkg/exp/eval/type.go:331: return big.NewRat(0, 1).SetFrac(numer,
> idealOne)
> new(big.Rat).SetInt(numer)
>
> http://codereview.appspot.com/1780042/diff/2001/3008#newcode1234
> src/pkg/exp/eval/type.go:1234: maxFloat32Val = big.NewRat(0,
> 1).SetFrac(numer, idealOne)
> s/big.NewRat(0, 1)/new(big.Rat)/
>
> apply also below
>
>
> http://codereview.appspot.com/1780042/show
>

[eds, 2010-07-13 00:49:04]

Hello gri (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2010-07-13 17:35:05]

A few more simplifications and adjustments to fit pkg/big idiomatic style.

http://codereview.appspot.com/1780042/diff/10001/8004
File src/pkg/exp/eval/expr.go (right):

http://codereview.appspot.com/1780042/diff/10001/8004#newcode21
src/pkg/exp/eval/expr.go:21: ratZero   = big.NewRat(0, 1)
remove ratZero - not needed if you use Sign() below

http://codereview.appspot.com/1780042/diff/10001/8004#newcode168
src/pkg/exp/eval/expr.go:168: if negErr != "" && val.Cmp(idealZero) == -1 {
instead: && val.Sign() < 0

http://codereview.appspot.com/1780042/diff/10001/8004#newcode1784
src/pkg/exp/eval/expr.go:1784: if (r.t.isInteger() &&
r.asIdealInt()().Cmp(idealZero) == 0) ||
use Sign() instead of Cmp

http://codereview.appspot.com/1780042/diff/10001/8004#newcode1785
src/pkg/exp/eval/expr.go:1785: (r.t.isFloat() && r.asIdealFloat()().Cmp(ratZero)
== 0) {
use Sign() instead of Cmp; then you don't need ratZero anymore

http://codereview.appspot.com/1780042/diff/10001/8005
File src/pkg/exp/eval/expr1.go (right):

http://codereview.appspot.com/1780042/diff/10001/8005#newcode232
src/pkg/exp/eval/expr1.go:232: v := v.asIdealInt()()
val := v.asIdealInt()()

http://codereview.appspot.com/1780042/diff/10001/8005#newcode233
src/pkg/exp/eval/expr1.go:233: val := v.Neg(v)
val.Neg(val)

(more idiomatic when using big)

http://codereview.appspot.com/1780042/diff/10001/8005#newcode240
src/pkg/exp/eval/expr1.go:240: val := v.Neg(v)
same here:

val := v.asIdealFloat()()
val.Neg(val)

http://codereview.appspot.com/1780042/diff/10001/8005#newcode267
src/pkg/exp/eval/expr1.go:267: val := v.Not(v)
and here:

val := v.asIdealInt()()
val.Not(val)

http://codereview.appspot.com/1780042/diff/10001/8009
File src/pkg/exp/eval/type.go (right):

http://codereview.appspot.com/1780042/diff/10001/8009#newcode249
src/pkg/exp/eval/type.go:249: numer = numer.Lsh(numer, bits)
just:

numer.Lsh(numer, bits)
numer.Sub(numer, idealOne)

http://codereview.appspot.com/1780042/diff/10001/8009#newcode319
src/pkg/exp/eval/type.go:319: numer = numer.Lsh(numer, bits-1)
just:

numer.Lsh(numer, bits-1)

http://codereview.appspot.com/1780042/diff/10001/8009#newcode329
src/pkg/exp/eval/type.go:329: numer = numer.Lsh(numer, bits-1)
same here:

numer.Lsh(numer, bits-1)
numer.Sub(numer, idealOne)

http://codereview.appspot.com/1780042/diff/10001/8009#newcode1233
src/pkg/exp/eval/type.go:1233: numer = numer.Lsh(numer, 127-23)
same here:

numer.Lsh(numer, 127-23)

http://codereview.appspot.com/1780042/diff/10001/8009#newcode1235
src/pkg/exp/eval/type.go:1235: numer = numer.SetInt64(0x1fffffffffffff)
numer.SetInt64(0x1fffffffffffff)

http://codereview.appspot.com/1780042/diff/10001/8009#newcode1236
src/pkg/exp/eval/type.go:1236: numer = numer.Lsh(numer, 1023-52)
numer.Lsh(numer, 1023-52)

[eds, 2010-07-13 23:12:14]

Hello gri (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2010-07-14 01:18:00]

http://codereview.appspot.com/1780042/diff/21001/22001
File src/pkg/exp/eval/Makefile (right):

http://codereview.appspot.com/1780042/diff/21001/22001#newcode34
src/pkg/exp/eval/Makefile:34: $(QUOTED_GOBIN)/$(LD) -o $@ $<;\
this should probably be

s/$@/generate/

[eds, 2010-07-14 02:39:50]

Hello gri (cc: golang-dev@googlegroups.com),

Please take another look.

[gri, 2010-07-14 16:39:44]

LGTM
thanks
- gri

On Tue, Jul 13, 2010 at 7:39 PM, <chickencha@gmail.com> wrote:

> Hello gri (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/1780042/show
>

[gri, 2010-07-14 16:40:05]

*** Submitted as http://code.google.com/p/go/source/detail?r=a479a9377fef ***

exp/eval: Converted from bignum to big

Also in this CL:
* Removed util.go, as its functionality is in big
* Removed some semicolons from the code generated by gen.go
* Added a generate target to Makefile
* Removed an outdated TODO from value.go

R=gri
CC=golang-dev
http://codereview.appspot.com/1780042

Committer: Robert Griesemer <gri@golang.org>