Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(28)

Issue 95460049: code review 95460049: math/rand: restore Go 1.2 value stream for Float32, Float64 (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 11 months ago by rsc
Modified:
9 years, 11 months ago
Reviewers:
r
CC:
iant, r, golang-codereviews
Visibility:
Public.

Description

math/rand: restore Go 1.2 value stream for Float32, Float64 CL 22730043 fixed a bug in these functions: they could return 1.0 despite documentation saying otherwise. But the fix changed the values returned in the non-buggy case too, which might invalidate programs depending on a particular stream when using rand.Seed(0) or when passing their own Source to rand.New. The example test says: // These tests serve as an example but also make sure we don't change // the output of the random number generator when given a fixed seed. so I think there is some justification for thinking we have promised not to change the values. In any case, there's no point in changing the values gratuitously: we can easily fix this bug without changing the values, and so we should. That CL just changed the test values too, which defeats the stated purpose, but it was just a comment. Add an explicit regression test, which might be a clearer signal next time that we don't want to change the values. Fixes issue 6721. (again) Fixes issue 8013.

Patch Set 1 #

Patch Set 2 : diff -r a129b2524679 https://code.google.com/p/go/ #

Patch Set 3 : diff -r c431c60edb20 https://code.google.com/p/go/ #

Patch Set 4 : diff -r ddf9fa84f55b https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+395 lines, -4 lines) Patch
M src/pkg/math/rand/example_test.go View 1 1 chunk +2 lines, -2 lines 0 comments Download
M src/pkg/math/rand/rand.go View 1 1 chunk +38 lines, -2 lines 0 comments Download
A src/pkg/math/rand/regress_test.go View 1 2 3 1 chunk +355 lines, -0 lines 0 comments Download

Messages

Total messages: 4
rsc
Hello iant, r (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 11 months ago (2014-05-19 13:36:00 UTC) #1
r
LGTM although the test could be avoid one level of reflection with some refactoring
9 years, 11 months ago (2014-05-19 15:07:26 UTC) #2
rsc
noted, but i'm going to leave the test as is for now to move on ...
9 years, 11 months ago (2014-05-19 16:30:09 UTC) #3
rsc
9 years, 11 months ago (2014-05-19 16:30:28 UTC) #4
*** Submitted as https://code.google.com/p/go/source/detail?r=0d7a1ac60a57 ***

math/rand: restore Go 1.2 value stream for Float32, Float64

CL 22730043 fixed a bug in these functions: they could
return 1.0 despite documentation saying otherwise.
But the fix changed the values returned in the non-buggy case too,
which might invalidate programs depending on a particular
stream when using rand.Seed(0) or when passing their own
Source to rand.New.

The example test says:
        // These tests serve as an example but also make sure we don't change
        // the output of the random number generator when given a fixed seed.
so I think there is some justification for thinking we have
promised not to change the values. In any case, there's no point in
changing the values gratuitously: we can easily fix this bug without
changing the values, and so we should.

That CL just changed the test values too, which defeats the
stated purpose, but it was just a comment.
Add an explicit regression test, which might be
a clearer signal next time that we don't want to change
the values.

Fixes issue 6721. (again)
Fixes issue 8013.

LGTM=r
R=iant, r
CC=golang-codereviews
https://codereview.appspot.com/95460049
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b