Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

math/big: Rat.Denom has side effects #33792

Closed
ericlagergren opened this issue Aug 22, 2019 · 10 comments
Closed

math/big: Rat.Denom has side effects #33792

ericlagergren opened this issue Aug 22, 2019 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.14

Comments

@ericlagergren
Copy link
Contributor

ericlagergren commented Aug 22, 2019 •
edited
Loading

From here: ericlagergren/decimal#138

My package divides (*Rat).Num by (*Rat).Denom to produce a decimal number. It turns out that (*Rat).Denom lazily sets its denominator, meaning it cannot be used concurrently.

Since the receiver is x not z, I assumed that the method simply read the denominator.

The receiver name should be updated to indicate side effects and/or the documentation for big.Rat should mention this side effect. I assume that the 7 years this has spent in the tree is too long to be reverted 😄. And, at any rate, not materializing the denominator can save allocations for whole numbers, which is a good thing.

I can send a CL if you want.
@bcmills bcmills added Documentation Issues describing a change to documentation. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 22, 2019
@bcmills bcmills added this to the Go1.14 milestone Aug 22, 2019
@bcmills
Copy link
Contributor

bcmills commented Aug 22, 2019

CC @griesemer

@griesemer
Copy link
Contributor

@ericlagergren Please send a CL. Assign to me for review. Thanks.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/192017 mentions this issue: math/big: document that Rat.Denom might modify the receiver

tomocy pushed a commit to tomocy/go that referenced this issue Sep 1, 2019
@ericlagergren @tomocy
math/big: document that Rat.Denom might modify the receiver
b08568d 
Fixes golang#33792

Change-Id: I306a95883c3db2d674d3294a6feb50adc50ee5d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/192017
Reviewed-by: Robert Griesemer <gri@golang.org>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this issue Sep 5, 2019
@ericlagergren @t4n6a1ka
math/big: document that Rat.Denom might modify the receiver
ad97e7e 
Fixes golang#33792

Change-Id: I306a95883c3db2d674d3294a6feb50adc50ee5d6
Reviewed-on: https://go-review.googlesource.com/c/go/+/192017
Reviewed-by: Robert Griesemer <gri@golang.org>
@rsc rsc mentioned this issue Oct 15, 2019
Closed
@griesemer
Copy link
Contributor

Reopening due to #34919
Should Denom() be allowed to have side-effects in the first place?
If we change this, we may break existing programs. See also #3521 and the respective test cases.

@griesemer griesemer reopened this Oct 15, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/201205 mentions this issue: math/big: make Rat accessors safe for concurrent use

gopherbot pushed a commit that referenced this issue Oct 15, 2019
@griesemer
math/big: make Rat accessors safe for concurrent use
898f9db 
Do not modify the underlying Rat denominator when calling
one of the accessors Float32, Float64; verify that we don't
modify the Rat denominator when calling Inv, Sign, IsInt, Num.

Fixes #34919.
Reopens #33792.

Change-Id: Ife6d1252373f493a597398ee51e7b5695b708df5
Reviewed-on: https://go-review.googlesource.com/c/go/+/201205
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@griesemer griesemer added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 16, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 16, 2019
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/202997 mentions this issue: math/big: normalize unitialized denominators ASAP

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/203059 mentions this issue: math/big: make Rat.Denom side-effect free

gopherbot pushed a commit that referenced this issue Oct 24, 2019
@griesemer
math/big: normalize unitialized denominators ASAP
4412181 
A Rat is represented via a quotient a/b where a and b are Int values.
To make it possible to use an uninitialized Rat value (with a and b
uninitialized and thus == 0), the implementation treats a 0 denominator
as 1.

For each operation we check if the denominator is 0, and then treat
it as 1 (if necessary). Operations that create a new Rat result,
normalize that value such that a result denominator 1 is represened
as 0 again.

This CL changes this behavior slightly: 0 denominators are still
interpreted as 1, but whenever we (safely) can, we set an uninitialized
0 denominator to 1. This simplifies the code overall.

Also: Improved some doc strings.

Preparation for addressing issue #33792.

Updates #33792.

Change-Id: I3040587c8d0dad2e840022f96ca027d8470878a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/202997
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed Documentation Issues describing a change to documentation. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 21, 2020
@dmitshur dmitshur mentioned this issue Mar 13, 2020
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/233323 mentions this issue: [release-branch.go1.13] math/big: make Rat.Denom side-effect free

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/233321 mentions this issue: [release-branch.go1.13] math/big: make Rat accessors safe for concurrent use

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/233322 mentions this issue: [release-branch.go1.13] math/big: normalize unitialized denominators ASAP

gopherbot pushed a commit that referenced this issue May 27, 2020
@griesemer @dmitshur
[release-branch.go1.13] math/big: make Rat accessors safe for concurr…
444ad95 
…ent use

Do not modify the underlying Rat denominator when calling
one of the accessors Float32, Float64; verify that we don't
modify the Rat denominator when calling Inv, Sign, IsInt, Num.

For #36689.
For #34919.
For #33792.

Change-Id: Ife6d1252373f493a597398ee51e7b5695b708df5
Reviewed-on: https://go-review.googlesource.com/c/go/+/201205
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/233321
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue May 27, 2020
@griesemer @dmitshur
[release-branch.go1.13] math/big: normalize unitialized denominators …
7f1ef49 
…ASAP

A Rat is represented via a quotient a/b where a and b are Int values.
To make it possible to use an uninitialized Rat value (with a and b
uninitialized and thus == 0), the implementation treats a 0 denominator
as 1.

For each operation we check if the denominator is 0, and then treat
it as 1 (if necessary). Operations that create a new Rat result,
normalize that value such that a result denominator 1 is represened
as 0 again.

This CL changes this behavior slightly: 0 denominators are still
interpreted as 1, but whenever we (safely) can, we set an uninitialized
0 denominator to 1. This simplifies the code overall.

Also: Improved some doc strings.

Preparation for addressing issue #33792.

For #36689.
For #33792.

Change-Id: I3040587c8d0dad2e840022f96ca027d8470878a0
Reviewed-on: https://go-review.googlesource.com/c/go/+/202997
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/233322
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
gopherbot pushed a commit that referenced this issue May 27, 2020
@griesemer @dmitshur
[release-branch.go1.13] math/big: make Rat.Denom side-effect free
90fe4a7 
A Rat is represented via a quotient a/b where a and b are Int values.
To make it possible to use an uninitialized Rat value (with a and b
uninitialized and thus == 0), the implementation treats a 0 denominator
as 1.

Rat.Num and Rat.Denom return pointers to these values a and b. Because
b may be 0, Rat.Denom used to first initialize it to 1 and thus produce
an undesirable side-effect (by changing the Rat's denominator).

This CL changes Denom to return a new (not shared) *Int with value 1
in the rare case where the Rat was not initialized. This eliminates
the side effect and returns the correct denominator value.

While this is changing behavior of the API, the impact should now be
minor because together with (prior) CL https://golang.org/cl/202997,
which initializes Rats ASAP, Denom is unlikely used to access the
denominator of an uninitialized (and thus 0) Rat. Any operation that
will somehow set a Rat value will ensure that the denominator is not 0.

Fixes #36689.
For #33792.
For #3521.

Change-Id: I0bf15ac60513cf52162bfb62440817ba36f0c3fc
Reviewed-on: https://go-review.googlesource.com/c/go/+/203059
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/233323
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
@golang golang locked and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
5 participants
@dmitshur @bcmills @ericlagergren @griesemer @gopherbot