code review 101750048: math: implement Nextafter32

math: implement Nextafter32

Provide Nextafter64 as alias to Nextafter.
For submission after the 1.3 release.

Fixes issue 8117.

[gri, 2014-05-28 21:33:35]

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

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

[adonovan, 2014-05-28 21:42:47]

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go
File src/pkg/math/all_test.go (right):

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
src/pkg/math/all_test.go:460: 4.979012489318848e+00,
I assume these numbers were generated by a similar process for the 64 bit case,
but it's not obvious what that process is.

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
src/pkg/math/all_test.go:1346: var vfnextafter32SC = [][2]float32{
What do "vf" and "SC" mean?

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go
File src/pkg/math/nextafter.go (right):

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go...
src/pkg/math/nextafter.go:33: func Nextafter64(x, y float64) (r float64) {
Is it really worth having both Nextafter64 and Nextafter?  The math package is
full of functions that assume float64 as the default floating-point format.

[gri, 2014-05-29 16:09:25]

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go
File src/pkg/math/all_test.go (right):

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
src/pkg/math/all_test.go:460: 4.979012489318848e+00,
On 2014/05/28 21:42:47, adonovan wrote:
> I assume these numbers were generated by a similar process for the 64 bit
case,
> but it's not obvious what that process is.

I don't know the processes used for any of the numbers in this test file but the
ones here: I simply created the bit patterns by hand, and then printed the
floats after creating them with Float32frombits.

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
src/pkg/math/all_test.go:1346: var vfnextafter32SC = [][2]float32{
On 2014/05/28 21:42:47, adonovan wrote:
> What do "vf" and "SC" mean?

I don't know. I am just following the existing pattern in this file.

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go
File src/pkg/math/nextafter.go (right):

https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go...
src/pkg/math/nextafter.go:33: func Nextafter64(x, y float64) (r float64) {
On 2014/05/28 21:42:47, adonovan wrote:
> Is it really worth having both Nextafter64 and Nextafter?  The math package is
> full of functions that assume float64 as the default floating-point format.

All the functions that assume float64 arguments and which don't have a 64 in
their name don't have a 32bit equivalent because it's not needed: one can always
convert the argument to 64bits.

All the functions which are different for 32 and 64 bits have the bit size in
their names. I am simply following that scheme. Since we cannot remove
Nextafter, I left it there. We could just not introduce Nextafter64 but that
seems also inconsistent.

The "bug" was there all along. Not feeling strongly, but it seems cleaner that
have the proper names plus a "deprecated" function for backward-compatibility.

[adonovan, 2014-05-29 18:08:16]

On 2014/05/29 16:09:25, gri wrote:
> https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go
> File src/pkg/math/all_test.go (right):
> 
>
https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
> src/pkg/math/all_test.go:460: 4.979012489318848e+00,
> On 2014/05/28 21:42:47, adonovan wrote:
> > I assume these numbers were generated by a similar process for the 64 bit
> case,
> > but it's not obvious what that process is.
> 
> I don't know the processes used for any of the numbers in this test file but
the
> ones here: I simply created the bit patterns by hand, and then printed the
> floats after creating them with Float32frombits.
> 
>
https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/all_test.go#...
> src/pkg/math/all_test.go:1346: var vfnextafter32SC = [][2]float32{
> On 2014/05/28 21:42:47, adonovan wrote:
> > What do "vf" and "SC" mean?
> 
> I don't know. I am just following the existing pattern in this file.
> 
> https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go
> File src/pkg/math/nextafter.go (right):
> 
>
https://codereview.appspot.com/101750048/diff/60001/src/pkg/math/nextafter.go...
> src/pkg/math/nextafter.go:33: func Nextafter64(x, y float64) (r float64) {
> On 2014/05/28 21:42:47, adonovan wrote:
> > Is it really worth having both Nextafter64 and Nextafter?  The math package
is
> > full of functions that assume float64 as the default floating-point format.
> 
> All the functions that assume float64 arguments and which don't have a 64 in
> their name don't have a 32bit equivalent because it's not needed: one can
always
> convert the argument to 64bits.
> 
> All the functions which are different for 32 and 64 bits have the bit size in
> their names. I am simply following that scheme. Since we cannot remove
> Nextafter, I left it there. We could just not introduce Nextafter64 but that
> seems also inconsistent.
> 
> The "bug" was there all along. Not feeling strongly, but it seems cleaner that
> have the proper names plus a "deprecated" function for backward-compatibility.

LGTM

[gri, 2014-06-11 16:09:40]

*** Submitted as https://code.google.com/p/go/source/detail?r=9f902e837883 ***

math: implement Nextafter32

Provide Nextafter64 as alias to Nextafter.
For submission after the 1.3 release.

Fixes issue 8117.

LGTM=adonovan
R=adonovan
CC=golang-codereviews
https://codereview.appspot.com/101750048

[rsc, 2014-06-11 18:21:59]

https://codereview.appspot.com/101750048/diff/80001/src/pkg/math/nextafter.go
File src/pkg/math/nextafter.go (right):

https://codereview.appspot.com/101750048/diff/80001/src/pkg/math/nextafter.go...
src/pkg/math/nextafter.go:49: // Nextafter is the same as Nextafter64.
Why not delete Nextafter64? Nearly everything in this package operates on
float64s. I would argue that is the default. Nextafter32 is a fine name but it
doesn't require deprecating all the mentions of plain Nextafter. It's churn
without a clear benefit.

[adonovan, 2014-06-11 18:24:15]

This question was asked and answered in the review:

---- quoting gri ----

On 2014/05/28 21:42:47, adonovan wrote:
> Is it really worth having both Nextafter64 and Nextafter?  The math
package is
> full of functions that assume float64 as the default floating-point
format.

All the functions that assume float64 arguments and which don't have a 64 in
their name don't have a 32bit equivalent because it's not needed: one can
always
convert the argument to 64bits.

All the functions which are different for 32 and 64 bits have the bit size
in
their names. I am simply following that scheme. Since we cannot remove
Nextafter, I left it there. We could just not introduce Nextafter64 but that
seems also inconsistent.

The "bug" was there all along. Not feeling strongly, but it seems cleaner
that
have the proper names plus a "deprecated" function for
backward-compatibility.


On 11 June 2014 14:21, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/101750048/diff/80001/src/
> pkg/math/nextafter.go
> File src/pkg/math/nextafter.go (right):
>
> https://codereview.appspot.com/101750048/diff/80001/src/
> pkg/math/nextafter.go#newcode49
> src/pkg/math/nextafter.go:49: // Nextafter is the same as Nextafter64.
> Why not delete Nextafter64? Nearly everything in this package operates
> on float64s. I would argue that is the default. Nextafter32 is a fine
> name but it doesn't require deprecating all the mentions of plain
> Nextafter. It's churn without a clear benefit.
>
> https://codereview.appspot.com/101750048/
>

[rsc, 2014-06-11 18:27:16]

Please err on the side of not causing and not causing duplication. If we
were starting from scratch then Nextafter32 and Nextafter64 would be fine.
But we're not starting from scratch, and renaming+deprecating Nextafter to
Nextafter64 causes work for users with no benefit, and perhaps worse, it
means that people who follow the directions to create Go 1.4 code that
doesn't compile in Go 1.3 and earlier.

General rule: if there is functionality under a given name, keep it there,
even if - given a clean slate - you'd pick a different name.

Russ

[gri, 2014-06-11 18:32:14]

Fair enough. As I said before, I am not feeling strongly, but personally I
prefer to start with clean (naming) layers, even at the cost of duplicating
names
That said, I'll remove it.
- gri


On Wed, Jun 11, 2014 at 11:27 AM, Russ Cox <rsc@golang.org> wrote:

> Please err on the side of not causing and not causing duplication. If we
> were starting from scratch then Nextafter32 and Nextafter64 would be fine.
> But we're not starting from scratch, and renaming+deprecating Nextafter to
> Nextafter64 causes work for users with no benefit, and perhaps worse, it
> means that people who follow the directions to create Go 1.4 code that
> doesn't compile in Go 1.3 and earlier.
>
> General rule: if there is functionality under a given name, keep it there,
> even if - given a clean slate - you'd pick a different name.
>
> Russ
>
>