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

Add CompareAndSwap and Swap to String, Error, Value, Deprecate CAS #111

Merged
merged 3 commits into from Aug 6, 2022

Conversation

eNV25
Copy link
Contributor

@eNV25 eNV25 commented Aug 5, 2022

Before opening your pull request, please make sure that you've:

  • updated the changelog if the change is user-facing;
  • added tests to cover your changes;
  • run the test suite locally (make test); and finally,
  • run the linters locally (make lint).

From #108 .

Add CompareAndSwap and Swap for String and Error. Deprecate CAS.

This is not done for Time since it is not meant to be compared.

I also have a commit bcb50e9, that deprecates CAS and adds CompareAndSwap to align with sync/atomic from go1.19. Since that commit depends on this one, I can only open a PR after this is merged.

@codecov
Copy link

codecov bot commented Aug 5, 2022

Codecov Report

Merging #111 (d2c8de7) into master (d15bdad) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           20        20           
  Lines          201       215   +14     
=========================================
+ Hits           201       215   +14     
Impacted Files Coverage Δ
error_ext.go 100.00% <ø> (ø)
string_ext.go 100.00% <ø> (ø)
bool.go 100.00% <100.00%> (ø)
duration.go 100.00% <100.00%> (ø)
error.go 100.00% <100.00%> (ø)
float32_ext.go 100.00% <100.00%> (ø)
float64_ext.go 100.00% <100.00%> (ø)
int32.go 100.00% <100.00%> (ø)
int64.go 100.00% <100.00%> (ø)
string.go 100.00% <100.00%> (ø)
... and 4 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Contributor

@abhinav abhinav left a comment

The change largely looks good minus minor comments.

However, I'm concerned about introducing CAS methods for String, Error, and Value and at the same time deprecating them in favor of CompareAndSwap.

Could we possibly skip that step? No CAS methods for things that didn't already have one?

error_test.go Outdated Show resolved Hide resolved
error_test.go Outdated Show resolved Hide resolved
string_test.go Outdated Show resolved Hide resolved
string_test.go Outdated Show resolved Hide resolved
value.go Outdated Show resolved Hide resolved
@eNV25
Copy link
Contributor Author

eNV25 commented Aug 5, 2022

However, I'm concerned about introducing CAS methods for String, Error, and Value and at the same time deprecating them in favor of CompareAndSwap.

I guess that means merging the commits, then removing the new CAS methods.

@eNV25
Copy link
Contributor Author

eNV25 commented Aug 5, 2022

then removing the new CAS methods.

I could do this by introducing a -compareandswap flag to the generator. The existing -cas flag would imply -compareandswap.

This is the only way to do this cleanly, I think. @abhinav What do you think?

Until feedback, I'll just update the PR to add the other commit that deprecates CAS. I can then implement the above later, after squashing all these commits.

@eNV25 eNV25 changed the title Add CAS and Swap to String, Error, Value Add CompareAndSwap and Swap to String, Error, Value, Deprecate CAS Aug 5, 2022
@abhinav
Copy link
Contributor

abhinav commented Aug 5, 2022

I could do this by introducing a -compareandswap flag to the generator. The existing -cas flag would imply -compareandswap.

This is the only way to do this cleanly, I think. @abhinav What do you think?

That sounds reasonable. The code generator is private so we can change its flags however we want.
-cas implies -compareandswap makes sense to me.

Thanks!

@eNV25 eNV25 requested a review from abhinav Aug 6, 2022
abhinav
abhinav approved these changes Aug 6, 2022
Copy link
Contributor

@abhinav abhinav left a comment

Thanks for making all the changes!

error_test.go Outdated Show resolved Hide resolved
error_test.go Outdated Show resolved Hide resolved
internal/gen-atomicint/main.go Outdated Show resolved Hide resolved
string_test.go Outdated Show resolved Hide resolved
string_test.go Outdated Show resolved Hide resolved
@abhinav abhinav merged commit d4bbbc8 into uber-go:master Aug 6, 2022
7 checks passed
@abhinav
Copy link
Contributor

abhinav commented Aug 6, 2022

Thanks!

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.

None yet

2 participants