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

Use proper atomics and ref counting #807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AreaZR
Copy link
Contributor

@AreaZR AreaZR commented Dec 16, 2023

Prefer __atomic_compare_exchange_n over __sync_bool_compare_and_swap.

I chose weak because we are looping and reading the value of old_value constantly anyway, so it would be better to have it weak.

Otherwise, it is equivalent to what it was before.

@ktoso
Copy link
Contributor

ktoso commented Dec 18, 2023

@swift-ci please test

@AreaZR
Copy link
Contributor Author

AreaZR commented Dec 18, 2023

@ktoso My bad! It compiled and ran fine on my machine but I found the mistake and fixed it.

Can we please try one more time?

@AreaZR
Copy link
Contributor Author

AreaZR commented Dec 18, 2023

@compnerd Can we please do a test?

@compnerd
Copy link
Member

I'm not sure that this is valuable enough to do really.

@rjmccall
Copy link

I'm not sure what the point of the atomics change is. The control flow change is non-obvious and totally unexplained in either comments or in the PR summary.

@AreaZR AreaZR force-pushed the atomic-3 branch 7 times, most recently from 2da1073 to 5acdd2b Compare December 24, 2023 20:01
Prefer __atomic_compare_exchange_n over __sync_bool_compare_and_swap.

I chose weak because we are looping and reading the value of old_value constantly anyway, so it would be better to have it weak.

Otherwise, it is equivalent to what it was before.
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.

4 participants