code review 217109: math: added ilogb, logb, remainder, tests and special c...

math: added ilogb, logb, remainder, tests and special conditions

Also added expm1_386 and remainder_386; shortened exp_386

[Charlie Dorian, 2010-02-24 00:32:16]

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

I'd like you to review this change.

[rsc, 2010-02-25 21:16:55]

looks pretty good.

http://codereview.appspot.com/217109/diff/20/26
File src/pkg/math/logb.go (right):

http://codereview.appspot.com/217109/diff/20/26#newcode7
src/pkg/math/logb.go:7: // Logb(x) returns the binary exponent of non-zero x.
Aren't these just wrappers for Frexp?
Are they worth having as separate functions?

http://codereview.appspot.com/217109/diff/20/26#newcode10
src/pkg/math/logb.go:10: //	Logb(+Inf) = +Inf
move two Inf cases next to each other.
Or maybe combine:

Logb(±Inf) = +Inf

below too

http://codereview.appspot.com/217109/diff/20/26#newcode12
src/pkg/math/logb.go:12: //	Ilogb(-Inf) = +Inf
Logb (not Ilogb)
next line too

http://codereview.appspot.com/217109/diff/20/26#newcode29
src/pkg/math/logb.go:29: //	Ilogb(+Inf) = MaxInt32
It's a little weird to have an int function 
returning MaxInt32, but there's no MaxInt.
Define math.MaxInt and math.MinInt?

http://codereview.appspot.com/217109/diff/20/27
File src/pkg/math/remainder.go (right):

http://codereview.appspot.com/217109/diff/20/27#newcode29
src/pkg/math/remainder.go:29: // Remainder returns the IEEE 754 remainder.
... remainder of x and y.

[Charlie Dorian, 2010-02-25 22:52:44]

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

Please take another look.

[Charlie Dorian, 2010-02-25 22:52:51]

Thanks for the review.

On Thu, Feb 25, 2010 at 4:16 PM, <rsc@golang.org> wrote:

> looks pretty good.
>
> http://codereview.appspot.com/217109/diff/20/26
> File src/pkg/math/logb.go (right):
>
> http://codereview.appspot.com/217109/diff/20/26#newcode7
> src/pkg/math/logb.go:7: // Logb(x) returns the binary exponent of
> non-zero x.
> Aren't these just wrappers for Frexp?
> Are they worth having as separate functions?
>
They sure look like it, especially because FLT_RADIX=2, but 1) they only
return one value,
2) they're part of C99 and POSIX, and 3) they were easy to do.

>
> http://codereview.appspot.com/217109/diff/20/26#newcode10
> src/pkg/math/logb.go:10: //     Logb(+Inf) = +Inf
> move two Inf cases next to each other.
> Or maybe combine:
>
> Logb(±Inf) = +Inf
>
Done.

>
> below too
>
Done.

>
> http://codereview.appspot.com/217109/diff/20/26#newcode12
> src/pkg/math/logb.go:12: //     Ilogb(-Inf) = +Inf
> Logb (not Ilogb)
> next line too
>
Done.

>
> http://codereview.appspot.com/217109/diff/20/26#newcode29
> src/pkg/math/logb.go:29: //     Ilogb(+Inf) = MaxInt32
> It's a little weird to have an int function
> returning MaxInt32, but there's no MaxInt.
>
I had a similar initial reaction, but I reasoned that the spec says
int may be either int32 or int64; MaxInt32 is much larger than
ilogb(MaxFloat64), so an error should readily apparent;
MaxInt32 is really just an untyped constant, not a type; and it's portable.

 Define math.MaxInt and math.MinInt?
>
Not yet needed. Otherwise const.go depends on the implementation.


> http://codereview.appspot.com/217109/diff/20/27
> File src/pkg/math/remainder.go (right):
>
> http://codereview.appspot.com/217109/diff/20/27#newcode29
> src/pkg/math/remainder.go:29: // Remainder returns the IEEE 754
> remainder.
> ... remainder of x and y.

Changed to ... "the IEEE 754 floating-point remainder of x/y." to be similar
to Fmod's initial comment line.

>
>
> http://codereview.appspot.com/217109/show
>

[Charlie Dorian, 2010-02-25 23:23:43]

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

Please take another look.

[Charlie Dorian, 2010-02-25 23:24:52]

Added TODO in logb.go for in-lining special cases.

On Thu, Feb 25, 2010 at 6:23 PM, <cldorian@gmail.com> wrote:

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

[rsc, 2010-03-04 01:30:10]

couple more nits, sorry.

Logb and Ilogb say "binary exponent" but I think it
should be "binary logarithm" to avoid confusion with
"binary exponential".

BenchmarkRemainder calls Fmod.

[Charlie Dorian, 2010-03-04 02:10:36]

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

Please take another look.

[Charlie Dorian, 2010-03-04 02:11:30]

Thanks for the review.  I appreciate it.

On Wed, Mar 3, 2010 at 8:30 PM, Russ Cox <rsc@golang.org> wrote:

> couple more nits, sorry.
>
> Logb and Ilogb say "binary exponent" but I think it
> should be "binary logarithm" to avoid confusion with
> "binary exponential".
>
Done

>
> BenchmarkRemainder calls Fmod.
>
Good eyes! Fixed.

[rsc, 2010-03-04 02:17:15]

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

math: added ilogb, logb, remainder, tests and special conditions

Also added expm1_386 and remainder_386; shortened exp_386

R=rsc
CC=golang-dev
http://codereview.appspot.com/217109

Committer: Russ Cox <rsc@golang.org>