code review 4044041: expvar: This change adds supports for floating point ex...

expvar: This change adds supports for floating point exported variables.

[josv, 2011-01-16 17:33:50]

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

I'd like you to review this change.

[dsymonds, 2011-01-16 21:04:45]

LGTM

Interested to hear how r/rsc feel about Add vs. AddInt.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:142: func (v *Map) AddInt(key string, delta int64) {
I don't think this method is necessary.

[r, 2011-01-16 21:50:55]

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:61: // Float is a 64-bit float variable, and satisfies
the Var interface.
s/, and/ that/

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:67: func (v *Float) String() string { return
strconv.Ftoa64(v.f, 'g', -1) }
these methods need doc comments.
many in this file don't have them but should.
i suggest putting them on your new functions and we can add the others in a
second round

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:142: func (v *Map) AddInt(key string, delta int64) {
agreed. although it adds consistency it supports a second naming convention.  i
i vote for just Add and AddFloat.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:146: func (v *Map) AddFloat(key string, delta float64)
{
needs a doc comment

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go
File src/pkg/expvar/expvar_test.go (right):

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go...
src/pkg/expvar/expvar_test.go:14: const DELTA = 0.00000000001
UPPERCASE isn't the go way

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go...
src/pkg/expvar/expvar_test.go:18: return math.Fabs(a-b) < DELTA
a better plan is to use floating point numbers without roundoff.
small integers and fractions of two are good.

[josv, 2011-01-16 22:42:25]

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

Please take another look.

[josv, 2011-01-16 22:43:30]

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:61: // Float is a 64-bit float variable, and satisfies
the Var interface.
On 2011/01/16 21:50:56, r wrote:
> s/, and/ that/

Done (changed it for Int and Map too).

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:67: func (v *Float) String() string { return
strconv.Ftoa64(v.f, 'g', -1) }
On 2011/01/16 21:50:56, r wrote:
> these methods need doc comments.
> many in this file don't have them but should.
> i suggest putting them on your new functions and we can add the others in a
> second round

Okay, adding them to my new functions. Once submitted I'll add them to the other
functions too in a new CL.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:142: func (v *Map) AddInt(key string, delta int64) {
On 2011/01/16 21:50:56, r wrote:
> agreed. although it adds consistency it supports a second naming convention. 
i
> i vote for just Add and AddFloat.

Done.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar.go#newc...
src/pkg/expvar/expvar.go:146: func (v *Map) AddFloat(key string, delta float64)
{
On 2011/01/16 21:50:56, r wrote:
> needs a doc comment

Done & see reply on previous comment.

[r2, 2011-01-16 22:52:54]

You didn't respond to my comments about expvar_test.go

-rob

[josv, 2011-01-16 23:39:18]

Yes, that's because I am a muppet. Sorry for that. :-(
Will get on it asap.

Jos

[josv, 2011-01-17 08:31:10]

Latest patch uploaded.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go
File src/pkg/expvar/expvar_test.go (right):

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go...
src/pkg/expvar/expvar_test.go:14: const DELTA = 0.00000000001
On 2011/01/16 21:50:56, r wrote:
> UPPERCASE isn't the go way

Deleted. See next reply.

http://codereview.appspot.com/4044041/diff/8001/src/pkg/expvar/expvar_test.go...
src/pkg/expvar/expvar_test.go:18: return math.Fabs(a-b) < DELTA
On 2011/01/16 21:50:56, r wrote:
> a better plan is to use floating point numbers without roundoff.
> small integers and fractions of two are good.

That is indeed a way better plan. Changed the code to use math that can be
accurately represented in our floating point scheme and removed these functions
again.

[rsc, 2011-01-18 00:19:54]

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:67: // Converts a floating point exported variable to a
suitable string representation.
delete; comment has wrong form,
and String method is so common that it usually
goes uncommented unless there's something
interesting to say.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:70: // Adds the specified delta to this floating point
exported variable.
wrong form
http://golang.org/doc/effective_go.html#commentary

// Add adds delta to v.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:77: // Sets the specified value as the new value of the
floating point exported variable.
Set sets v to value.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:145: // Adds the delta to the floating point variable
stored in this map under the specified key.
// AddFloat adds delta to the *Float value stored under the given map key.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:194: // Converts the out of this floating point
function to a suitable string representation.
delete

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:241: // Creates a new floating point exported variable.
delete

[josv, 2011-01-18 09:00:17]

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go
File src/pkg/expvar/expvar.go (right):

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:67: // Converts a floating point exported variable to a
suitable string representation.
On 2011/01/18 00:19:54, rsc wrote:
> delete; comment has wrong form,
> and String method is so common that it usually
> goes uncommented unless there's something
> interesting to say.
> 
> 

Done.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:70: // Adds the specified delta to this floating point
exported variable.
On 2011/01/18 00:19:54, rsc wrote:
> wrong form
> http://golang.org/doc/effective_go.html#commentary
> 
> // Add adds delta to v.

Done, and thanks for the link.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:77: // Sets the specified value as the new value of the
floating point exported variable.
On 2011/01/18 00:19:54, rsc wrote:
> Set sets v to value.

Done.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:145: // Adds the delta to the floating point variable
stored in this map under the specified key.
On 2011/01/18 00:19:54, rsc wrote:
> // AddFloat adds delta to the *Float value stored under the given map key.

Done.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:194: // Converts the out of this floating point
function to a suitable string representation.
On 2011/01/18 00:19:54, rsc wrote:
> delete

Done.

http://codereview.appspot.com/4044041/diff/28001/src/pkg/expvar/expvar.go#new...
src/pkg/expvar/expvar.go:241: // Creates a new floating point exported variable.
On 2011/01/18 00:19:54, rsc wrote:
> delete

Done.

[rsc, 2011-01-18 15:15:07]

LGTM

[rsc, 2011-01-18 16:24:45]

*** Submitted as 4896d9611bda ***

expvar: add Float

R=r, rsc, dsymonds
CC=golang-dev
http://codereview.appspot.com/4044041

Committer: Russ Cox <rsc@golang.org>