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

multi: Use atomic types in exported modules. #3054

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jholdstock
Copy link
Member

go 1.19 introduced new atomic types which hide the underlying primitive types so that all accesses are forced to use the atomic APIs. This makes the code less prone to human error and a bit less verbose.

Extracted from #3053 which was originally a repo-wide change, this PR includes just the exported modules.

@davecgh
Copy link
Member

davecgh commented Feb 13, 2023

Copying the relevant discussion from #3053:

From @davecgh:

...

Note that this change would have to bump the relevant go.mod files to require Go 1.19 because the types were not introduced until Go 1.19.

... I'd prefer not to needlessly require newer Go versions for the other modules which hoist everyone else forward (at elast yet) unless there is a really good reason and I don't think this qualifies.

From @jrick:

Go <=1.18 no longer receives security patches. I'm fine leaving it behind.

From @davecgh:

That's a choice each consumer has to make with their own projects, imo. I don't think it's fair to force our own policies on everyone who uses our modules just to get updated versions of them in general. As someone who has been bitten many times by being on the receiving end of such policies, I'm particularly sensitive to it.

For any cases where there is a legitimate security issue resolved that genuinely requires a new version, we'd obviously bump them.

It might also be reasonable to follow the Go bootstrap toolchain as a minimum here since you won't even be able to build the relevant version of Go without that version.

@jrick
Copy link
Member

jrick commented Feb 13, 2023

It might also be reasonable to follow the Go bootstrap toolchain as a minimum here since you won't even be able to build the relevant version of Go without that version.

The bootstrap is only intended for building a more recent Go. There is no reason to build any other project from it.

connmgr/connmanager.go Outdated Show resolved Hide resolved
connmgr/connmanager.go Outdated Show resolved Hide resolved
connmgr/connmanager.go Outdated Show resolved Hide resolved
connmgr/connmanager.go Outdated Show resolved Hide resolved
connmgr/connmanager.go Outdated Show resolved Hide resolved
@jholdstock
Copy link
Member Author

Rebased after #3055

connmgr/connmanager.go Outdated Show resolved Hide resolved
connmgr/connmanager.go Show resolved Hide resolved
peer/peer.go Show resolved Hide resolved
@jholdstock
Copy link
Member Author

Rebased and updated for 2024

@davecgh davecgh modified the milestones: 1.9.0, Future Version May 9, 2024
@teknico
Copy link
Contributor

teknico commented May 10, 2024

The CI pipeline is only using Go 1.21 and 1.22, so building on 1.19 and 1.20 is promised but not checked.

Adding these two older versions to the CI would double the time it takes to run the tests on each PR update.

And even then, building the non-main modules on 1.17 and 1.18 would still not be checked.

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

Successfully merging this pull request may close these issues.

6 participants