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.

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

Remove methods duplicated in go1.19 sync/atomic #119

Open
3 tasks
eNV25 opened this issue Aug 12, 2022 · 3 comments
Open
3 tasks

Remove methods duplicated in go1.19 sync/atomic #119

eNV25 opened this issue Aug 12, 2022 · 3 comments

Comments

@eNV25
Copy link
Contributor

eNV25 commented Aug 12, 2022

Since the go1.19 update, there are lots of duplicate functionality between this package and sync/atomic. This duplication is currently needed because go1.18 is still supported.

sync/atomic has the following types that are duplicated here:

  • Bool
  • Int32
  • Int64
  • Pointer[T]
  • Uint32
  • Uint64
  • Uintptr

All types in sync/atomic have the following methods:

  • CompareAndSwap
  • Load
  • Store
  • Swap

Integer-like types have the following, in addition to the above:

  • Add

But there is still some functionality in here go.uber.org/atomic that is not there in sync/atomic.

All thing considered, after go1.18 becomes unsupported, we could do the following:

  • Update gen-atomicint to embed sync/atomic types. Only the following additional methods
    need to be kept.
    - CAS (Deprecated)
    - Dec
    - Inc
    - Sub
    - String
    - MarshallJSON
    - UnmarshallJSON
  • Update Bool to embed sync/atomic type. Only the following additional methods
    need to be kept.
    - CAS (Deprecated)
    - String
    - Toggle
    - MarshallJSON
    - UnmarshallJSON
  • Update Pointer[T] to embed sync/atomic type.

I just wanted to document all this stuff before I forget.

@abhinav
Copy link
Collaborator

abhinav commented Aug 12, 2022

This sounds reasonable. With Pointer[T] and Go 1.19, I considered embedding but opted against it so that we still had control of the exact API surface of the type exported by this package. There were two reasons to do this:

  • Guard against API updates in the standard library that conflict with our APIs. I think the risk of that is pretty low, though, so I think this point is largely moot.
  • Documentation accessibility. For a full list of methods, users will have to click through to the embedded type rather than have them listed on the documentation page.

I'm not opposed to embedding, but I do want to weigh the documentation accessibility issue against it.

Regardless, whether we embed or not, we should at least reuse the standard library types' implementations now.

@eNV25
Copy link
Contributor Author

eNV25 commented Aug 12, 2022

I am fine with both embedding and wrapping. I don't think it makes much of a difference in practice.

If we decide to embed, we should explain that in the docs like for Value.

Else if we decide to wrap, we should do that for Value as well.

@hrissan
Copy link

hrissan commented Jun 26, 2024

Wrappers are mandatory at least for some types, because if you look into standard package, you'll find that there is special code ensuring alignment of int64 values on platforms that require it.

std:
type Int64 struct {
_ noCopy
_ align64
v int64
}

Without that align64, code is simply incorrect and relies on the chance to be correctly aligned.

uber:
type Int64 struct {
_ nocmp
v int64
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants