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

Replace implementation of AtomicBool etc. by atomic from the standard library #1949

Open
ahoppen opened this issue Jan 22, 2025 · 8 comments · May be fixed by #1969
Open

Replace implementation of AtomicBool etc. by atomic from the standard library #1949

ahoppen opened this issue Jan 22, 2025 · 8 comments · May be fixed by #1969

Comments

@ahoppen
Copy link
Member

ahoppen commented Jan 22, 2025

We should be able to raise the deployment target of SourceKit-LSP to macOS 14.0 and thus use the atomics from the standard library.

@ahoppen
Copy link
Member Author

ahoppen commented Jan 23, 2025

Synced to Apple’s issue tracker as rdar://143438616

@mustiikhalil
Copy link
Contributor

I wonder if replacing it with a ManagedAtomic<Type> would be more than enough or does it have to be UnsafeAtomic?

@ahoppen
Copy link
Member Author

ahoppen commented Jan 27, 2025

I meant ManagedAtomic by the atomic types in the standard library.

@mustiikhalil
Copy link
Contributor

Perfect! I can take a look at this by the end of this week

@mustiikhalil
Copy link
Contributor

Do we want to always import swift-atomics in each and every framework? or should we write a wrapper around it?

example:

struct Atomic<T> {
  let lock: ManagedAtomic<T>

var value: T? {
  get { lock.load(ordering: ....)  }
  set { lock.store(newValue, ordering: ....) }
}

The code above will replace somthing like this:

let lock = ManagedAtomic(false)
/// some code
lock.store(v, ordering: ...)
/// more code
if lock.load(ordering: ....) {

}

@ahoppen
Copy link
Member Author

ahoppen commented Feb 4, 2025

I was thinking to use the Atomic type that’s available in the standard library: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0410-atomics.md

@mustiikhalil
Copy link
Contributor

mustiikhalil commented Feb 4, 2025

Oh I didn't know that existed within the Synchronization framework.

At the same time wouldn't that only work on swift 6 and above? Is deprecating swift 5.10 support in the libraries roadmap?

https://github.com/swiftlang/swift/blob/c14561fba3c057a247f5bcaa514536cd0d22102b/stdlib/public/Synchronization/Atomics/Atomic.swift#L20

(And it seems that that Atomics is based on macOS 15, not sure yet. Still trying to figure out why Xcode is misbehaving)

@ahoppen
Copy link
Member Author

ahoppen commented Feb 4, 2025

Yes, we can raise the deployment target to macOS 15 on main, so we can use Synchronization.

@mustiikhalil mustiikhalil linked a pull request Feb 5, 2025 that will close this issue
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 a pull request may close this issue.

2 participants