code review 976041: big: Create type nat

big: Create type nat

Changed most of the functions in nat.go to methods on nat.

[eds, 2010-04-22 03:41:20]

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

I'd like you to review this change.

[gri, 2010-04-22 18:05:18]

This CL makes me a bit nervous. The reason why I didn't write these functions as
methods (despite the TODO) was to emphasize that these are raw "algorithms"
operating on raw []Words. It's very tempting now to export Nat as well (which I
am not sure is the right thing to do). That said, I admit the some of the code
looks better, it's certainly shorter.

Do you have any compelling reasons for the change besides my TODO?

Some comments in the meantime.
- gri

http://codereview.appspot.com/976041/diff/3001/4004
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/976041/diff/3001/4004#newcode51
src/pkg/big/nat.go:51: func makeN(z nat, m int, clear bool) nat {
this should be a method "make"

http://codereview.appspot.com/976041/diff/3001/4004#newcode70
src/pkg/big/nat.go:70: func newN(z nat, x uint64) nat {
same here: should be method "new"

http://codereview.appspot.com/976041/diff/3001/4004#newcode267
src/pkg/big/nat.go:267: panic("Divide by zero undefined")
this should be "division by zero" to match the panic on lune 249

[eds, 2010-04-22 18:30:11]

There's no real pressing need for this. I'm basically trying to develop this
package more so that we can deprecate bignum once and for all. If creating the
nat type isn't something we want to do (which is fine by me), then we should
remove those todos.

As for exporting nat, I'm undecided, but leaning toward not exporting it. The
main argument against exporting is that it clutters up the interface, as we'd
not only have one more public type and its methods, but we'd also be expected to
provide methods converting back and forth between Int and Nat.

There aren't many things you could do with Nat that you can't do with Int. The
main thing I can think of is that bitwise operations on Nat would give different
results, but I'm not sure how useful that is. It would also give us a way to
ensure that the argument we're receiving is positive when it makes sense. (For
example, we'll probably be getting some kind of Binomial function at some point,
where it doesn't make sense for k to be negative.)

http://codereview.appspot.com/976041/diff/3001/4004
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/976041/diff/3001/4004#newcode51
src/pkg/big/nat.go:51: func makeN(z nat, m int, clear bool) nat {
On 2010/04/22 18:05:19, gri wrote:
> this should be a method "make"
> 

Should it also be a method on nat? This is something I was debating because
there are quite a few places where the current code would translate into
nat(nil).make(...), which looks very odd to me. At the same time, there are a
few other functions that have calls like this that I did convert. I'm not
entirely comfortable with either choice.

http://codereview.appspot.com/976041/diff/3001/4004#newcode70
src/pkg/big/nat.go:70: func newN(z nat, x uint64) nat {
On 2010/04/22 18:05:19, gri wrote:
> same here: should be method "new"

Same question as above, although I think there are fewer calls where z == nil
for this one.

[gri, 2010-04-22 19:11:58]

I starred at this some more and I think we should do the conversion. The code
looks better, is shorter and easier to read.

There's some comments that are unrelated to this CL, fine if you want to address
them separately.

- Robert

http://codereview.appspot.com/976041/diff/3001/4004
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/976041/diff/3001/4004#newcode37
src/pkg/big/nat.go:37: // TODO(gri) - decide if type 'nat' should be exported
I think this should be a no. Please just remove the TODO.

http://codereview.appspot.com/976041/diff/3001/4004#newcode51
src/pkg/big/nat.go:51: func makeN(z nat, m int, clear bool) nat {
On 2010/04/22 18:30:11, eds wrote:
> On 2010/04/22 18:05:19, gri wrote:
> > this should be a method "make"
> > 
> 
> Should it also be a method on nat? This is something I was debating because
> there are quite a few places where the current code would translate into
> nat(nil).make(...), which looks very odd to me. At the same time, there are a
> few other functions that have calls like this that I did convert. I'm not
> entirely comfortable with either choice.

I think so; this will make it consistent. nat(nil).make(...) is fine. If you
prefer, introduce a temporary

var t nat
t.make(...)

may be clearer. In fact, in some places that temporary exists already and you
don't even need anything special (see e.g. divLargeNN).

Perhaps 'make' is not a good name anymore, but we can decide that separately.

http://codereview.appspot.com/976041/diff/3001/4004#newcode70
src/pkg/big/nat.go:70: func newN(z nat, x uint64) nat {
On 2010/04/22 18:30:11, eds wrote:
> On 2010/04/22 18:05:19, gri wrote:
> > same here: should be method "new"
> 
> Same question as above, although I think there are fewer calls where z == nil
> for this one.

same answer

http://codereview.appspot.com/976041/diff/3001/4004#newcode304
src/pkg/big/nat.go:304: u = makeN(nil, len(uIn)+1, true) // uIn is an alias for
z2
this will become: u = u.make(...)

http://codereview.appspot.com/976041/diff/3001/4004#newcode542
src/pkg/big/nat.go:542: func (dst nat) shiftLeft(src nat, n uint) {
dst and nat should really be z and x, to be consistent with the conventions in
this file. Also, is there a good reason for not return the result? (again,
matching conventions in this file)

http://codereview.appspot.com/976041/diff/3001/4004#newcode561
src/pkg/big/nat.go:561: func (dst nat) shiftRight(src nat, n uint) {
same comment as for shiftLeft

[eds, 2010-04-22 23:16:04]

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

Please take another look.

[gri, 2010-04-22 23:50:36]

One minor knit.

http://codereview.appspot.com/976041/diff/14001/15004
File src/pkg/big/nat.go (right):

http://codereview.appspot.com/976041/diff/14001/15004#newcode538
src/pkg/big/nat.go:538: // To avoid losing the top n bits, dst should be sized
so that
replace src and dst with x and z in comment

[eds, 2010-04-22 23:53:48]

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

Please take another look.

[gri, 2010-04-22 23:57:17]

LGTM.
Thanks.
- Robert


On Thu, Apr 22, 2010 at 4:53 PM, <chickencha@gmail.com> wrote:

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

[gri, 2010-04-22 23:57:34]

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

big: Create type nat

Changed most of the functions in nat.go to methods on nat.

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

Committer: Robert Griesemer <gri@golang.org>