Basics: extract darwinCacheDirectories from TSCUtility
#6011
Conversation
compnerd
requested review from
abertelrud,
neonichu,
tomerd and
elsh
as code owners
|
@swift-ci please smoke test |
This marks the `Platform` type as deprecated. The functionality has been split up into swift-package-manager and sourcekit-lsp as appropriate. There remain no users of this at this point. apple/sourcekit-lsp#686 apple/swift-package-manager#6011
Sources/Basics/Sandbox.swift
Outdated
| @@ -55,6 +53,29 @@ public enum Sandbox { | |||
| // MARK: - macOS | |||
|
|
|||
| #if os(macOS) | |||
| fileprivate let threadSafeDarwinCacheDirectories: ThreadSafeArrayStore<AbsolutePath> = { | |||
There was a problem hiding this comment.
the use of ThreadSafeArrayStore here was for caching and we loose the cache with this change. that may be okay, but then we dont need ThreadSafeArrayStore
@abertelrud wdyt about loosing the cache?
There was a problem hiding this comment.
We do retain the caching in some aspect - the value will be computed once and re-used rather than computed each time. The idea of replacing the ThreadSafeArrayStore<AbsolutePath> with [AbsolutePath] however still stands and would also simplify this further. I think that need to wait for @abertelrud to chime in on that.
There was a problem hiding this comment.
I do think it's a good idea to compute this only once (i.e. keep the cache). Based on @compnerd's comment I do see that the value gets computed only once, but it does get computed once for each Sandbox instance, so it's not cached as much as it was. It also seems that the change looses the thread safety aspect.
Couldn't it be implemented as a static, as it was before, where the contents of ThreadSafeArrayStore are what get cached? i.e. keep the property as static though no longer on an extension of TSCUtility.Platform.
There was a problem hiding this comment.
Actually, are the initializers of Swift statics atomic already? If so, do we need ThreadSafeArrayStore at all here?
There was a problem hiding this comment.
@abertelrud - that is my understanding, that the initialisers are atomic, which is why I removed the locking in the first place. If that is not the case, we should restore that. As to the static bit, this is effectively so - the variable is a global variable, and thus does not need to be marked static.
There was a problem hiding this comment.
A static property of the Sandbox type makes sense to me.
There was a problem hiding this comment.
I'm not sure that moving it into Sandbox makes it any more encapsulated, but does namespace it. I don't see the value, but I can move it nonetheless. I'll remove the now unnecessary use of ThreadSafeArrayStore.
There was a problem hiding this comment.
I have no strong opinion about whether it's a fileprivate global or a static inside Sandbox, so whatever @tomerd and you agree on is fine with me.
There was a problem hiding this comment.
Actually, that is even more odd - the value is used in a single free-standing function only. I'm leaving it currently as a global until @tomerd responds.
There was a problem hiding this comment.
I prefer it inside Sandbox, but won't hold this back
This function is used solely in SPM and is the last portion of `TSCUtility.Platform` in use. Move the implementation into the single use site to allow us to deprecate the interface in swift-tools-support-core.
|
@swift-ci smoke test |
This marks the `Platform` type as deprecated. The functionality has been split up into swift-package-manager and sourcekit-lsp as appropriate. There remain no users of this at this point. apple/sourcekit-lsp#686 apple/swift-package-manager#6011
This function is used solely in SPM and is the last portion of
TSCUtility.Platformin use. Move the implementation into the single use site to allow us to deprecate the interface in swift-tools-support-core.