Disallow comparisons #72
Comments
|
Not allowing comparisons is a good idea.
Agree. |
abhinav
added a commit
that referenced
this issue
May 12, 2020
It is currently possible to make non-atomic comparisons of atomic
values:
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(*x == *y)
This is undesirable because it loads the value in a non-atomic way. The
correct method is,
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(x.Load() == y.Load())
To prevent this, disallow comparison of atomic values by embedding an
uncomparable array into atomics. The empty array adds no runtime cost,
and does not increase the in-memory size of the structs. Inspired by
[go4.org/mem#RO][1].
[1]: https://github.com/go4org/mem/blob/3dbcd07079b881f9bedb802b4099856c1e267fa1/mem.go#L42
Note that the Value struct, which embeds `"sync/atomic".Value` was also
changed. This will break usages in the following form, but that's
acceptable because unkeyed struct literals are among the exceptions
stated in the [Go 1 compatibility expectations][2].
import (
syncatomic "sync/atomic"
"go.uber.org/atomic"
)
atomic.Value{syncatomic.Value{..}}
[2]: https://golang.org/doc/go1compat#expectations
Resolves #72
abhinav
added a commit
that referenced
this issue
May 12, 2020
It is currently possible to make non-atomic comparisons of atomic
values:
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(*x == *y)
This is undesirable because it loads the value in a non-atomic way. The
correct method is,
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(x.Load() == y.Load())
To prevent this, disallow comparison of atomic values by embedding an
uncomparable array into atomics. The empty array adds no runtime cost,
and does not increase the in-memory size of the structs. Inspired by
[go4.org/mem#RO][1].
[1]: https://github.com/go4org/mem/blob/3dbcd07079b881f9bedb802b4099856c1e267fa1/mem.go#L42
Note that the Value struct, which embeds `"sync/atomic".Value` was also
changed. This will break usages in the following form, but that's
acceptable because unkeyed struct literals are among the exceptions
stated in the [Go 1 compatibility expectations][2].
import (
syncatomic "sync/atomic"
"go.uber.org/atomic"
)
atomic.Value{syncatomic.Value{..}}
[2]: https://golang.org/doc/go1compat#expectations
Resolves #72
abhinav
added a commit
that referenced
this issue
May 12, 2020
It is currently possible to make non-atomic comparisons of atomic
values:
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(*x == *y)
This is undesirable because it loads the value in a non-atomic way. The
correct method is,
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(x.Load() == y.Load())
To prevent this, disallow comparison of atomic values by embedding an
uncomparable array into atomics. The empty array adds no runtime cost,
and does not increase the in-memory size of the structs. Inspired by
[go4.org/mem#RO][1].
[1]: https://github.com/go4org/mem/blob/3dbcd07079b881f9bedb802b4099856c1e267fa1/mem.go#L42
Note that the Value struct, which embeds `"sync/atomic".Value` was also
changed. This will break usages in the following form, but that's
acceptable because unkeyed struct literals are among the exceptions
stated in the [Go 1 compatibility expectations][2].
import (
syncatomic "sync/atomic"
"go.uber.org/atomic"
)
atomic.Value{syncatomic.Value{..}}
[2]: https://golang.org/doc/go1compat#expectations
Resolves #72
abhinav
added a commit
that referenced
this issue
May 12, 2020
It is currently possible to make non-atomic comparisons of atomic
values:
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(*x == *y)
This is undesirable because it loads the value in a non-atomic way. The
correct method is,
x := atomic.NewInt32(1)
y := atomic.NewInt32(1)
fmt.Println(x.Load() == y.Load())
To prevent this, disallow comparison of atomic values by embedding an
uncomparable array into atomics. The empty array adds no runtime cost,
and does not increase the in-memory size of the structs. Inspired by
[go4.org/mem#RO][1].
[1]: https://github.com/go4org/mem/blob/3dbcd07079b881f9bedb802b4099856c1e267fa1/mem.go#L42
Note that the Value struct, which embeds `"sync/atomic".Value` was also
changed. This will break usages in the following form, but that's
acceptable because unkeyed struct literals are among the exceptions
stated in the [Go 1 compatibility expectations][2].
import (
syncatomic "sync/atomic"
"go.uber.org/atomic"
)
atomic.Value{syncatomic.Value{..}}
[2]: https://golang.org/doc/go1compat#expectations
Resolves #72
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Currently, it's possible to do,
This accesses the value of the integer in a non-atomic way.
We should consider disabling this by adding a
_ [0]func()field to theatomic structs. This would add no runtime cost and disable struct-level
comparison of the values. (Ref: https://github.com/go4org/mem/blob/3dbcd07079b881f9bedb802b4099856c1e267fa1/mem.go#L42.)
Worth considering that this could constitute a breaking change. An argument
for in favor of the change is that the comparison was incorrect usage in the
first place, so it's not breaking to disallow it.
The text was updated successfully, but these errors were encountered: