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

Recommend the stdlib atomic types instead of go.uber.org/atomic #3

Merged
merged 3 commits into from
Feb 14, 2025

Conversation

autarch
Copy link
Collaborator

@autarch autarch commented Feb 10, 2025

Uber's package predates the new types added in Go 1.19. The new stdlib types provide more or less the same functionality as Uber's.

@autarch autarch changed the base branch from master to mongodb-ify February 10, 2025 20:27
@autarch autarch force-pushed the atomics branch 2 times, most recently from f486e62 to 06bae45 Compare February 10, 2025 21:27
Base automatically changed from mongodb-ify to master February 13, 2025 16:20
@autarch autarch marked this pull request as ready for review February 13, 2025 16:23
src/atomic.md Outdated
[sync/atomic]: https://pkg.go.dev/sync/atomic
<!--

TODO: It'd be nice to have a shared library that provided a typed atomic value instead of having
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see that file is proposing a typed value, not a pointer. That being said, could we actually leave out discussion like this from within the files? Might be better placed elsewhere like an issue or ticket, just so these don't add up and become harder to action.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seconded on issues

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this.

style.md Outdated

<!--

TODO: It'd be nice to have a shared library that provided a typed atomic value instead of having
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that style.md is just a generated cat of all the individual files in src. I will update .gitattributes so it shows up as a generated file in the PR Files Changed view.

@autarch autarch requested review from pjjw and edaniels February 13, 2025 18:48
Copy link
Collaborator

@edaniels edaniels left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@autarch
Copy link
Collaborator Author

autarch commented Feb 14, 2025

@pjjw Ping ...

Copy link
Collaborator

@pjjw pjjw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh saw eric's LGTM and agree! let me just register that intent properly

@autarch autarch merged commit 66553ff into master Feb 14, 2025
1 check passed
@autarch autarch deleted the atomics branch February 14, 2025 16:53
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.

3 participants