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

darwin: Use atomic for in_reenumerate flag #1399

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tormodvolden
Copy link
Contributor

No description provided.

@tormodvolden tormodvolden marked this pull request as draft December 22, 2023 18:27
Signed-off-by: Tormod Volden <debian.tormod@gmail.com>
@seanm
Copy link
Contributor

seanm commented Dec 22, 2023

Thanks! This is indeed what I had in mind, but didn't have a chance to type it up because of end-of-year work craziness.

Let's see if this works. If so, will need to see how far back these C atomics are supported on macOS, if not far, there are Apple-specific variants that could be used.

@mcuee mcuee added the macOS label Dec 23, 2023
@mcuee
Copy link
Member

mcuee commented Dec 23, 2023

@tormodvolden
Copy link
Contributor Author

Thanks for testing! Maybe this still can be seen as an improvement and considered for inclusion, after 1.0.27 some time.

@mcuee
Copy link
Member

mcuee commented Dec 23, 2023

Thanks for testing! Maybe this still can be seen as an improvement and considered for inclusion, after 1.0.27 some time.

If the PR is reviewed to be correct and fixes one existing code defect, I think we can still merge ti before 1.0.27 release, even though it does not fix issue #1386.

@seanm
Copy link
Contributor

seanm commented Jan 2, 2024

I think this change is correct, and makes the code more correct, but I think there's an even bigger threading issue with nearby code. I'd suggest holding off merging this until I dig deeper...

@tormodvolden
Copy link
Contributor Author

I hope we can find a surgical fix for #1386 and soon release 1.0.27 which is otherwise well tested, then apply this and all your recent darwin merge requests afterwards.

@seanm
Copy link
Contributor

seanm commented Jan 2, 2024

I hope we can find a surgical fix for #1386

Agreed.

and soon release 1.0.27 which is otherwise well tested, then apply this and all your recent darwin merge requests afterwards.

Some of my patches I think should be considered for 1.0.27, I'll point them out...

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

Successfully merging this pull request may close these issues.

None yet

3 participants