Only use FoundationEssentials if available#749
Conversation
natecook1000
left a comment
natecook1000
left a comment
There was a problem hiding this comment.
Thanks for working on this, @t089! A couple notes below – I'll look at getting rid of the lock implementation altogether in a different PR.
| #if canImport(FoundationEssentials) | ||
| func shellEscapeForSingleQuotedString(iterationCount: UInt64 = 1) -> Self { | ||
| iterationCount == 0 | ||
| ? self | ||
| : replacing("'", with: "'\\''") | ||
| .shellEscapeForSingleQuotedString(iterationCount: iterationCount - 1) | ||
| } | ||
| #else |
There was a problem hiding this comment.
Is this canImport check necessary? replacing(_:with:) comes from the Swift standard library (via _StringProcessing), not FoundationEssentials. We may be able to do away with the other branch on these altogether.
There was a problem hiding this comment.
for all the branches where the result is the same, we should remove the conditional code and use the stdlib version theres no need to use the variants from Foundation.
There was a problem hiding this comment.
The problem is the stdlib version. On macOS this is only available on newer macOS versions. But since on macOS you anyhow have foundation I chose this route.
There was a problem hiding this comment.
I'd personally prefer manually backdeploying these stdlib functions then.
There was a problem hiding this comment.
@rauhul Should I try to copy an implementation from _StringProcessing? Alternatively, is there something like #if canImport(SwiftStdlib, 6.0) that we could use here?
| // If the type is named something like "TransformOptions", we only want | ||
| // to use "transform" as the command name. | ||
| if let optionsRange = name.range(of: "_options"), | ||
| if let optionsRange = name._range(of: "_options"), |
There was a problem hiding this comment.
Try firstRange(of:) here for the non-Foundation version, instead of copying in an implementation.
There was a problem hiding this comment.
'firstRange(of:)' is only available in macOS 13.0 or newer
There was a problem hiding this comment.
@natecook1000 what is your preferred course of action? Keep the copied implementation here?
| /// of lock is safe to use with `libpthread`-based threading models, such as the | ||
| /// one used by NIO. On Windows, the lock is based on the substantially similar | ||
| /// `SRWLOCK` type. | ||
| public struct NIOLock { |
There was a problem hiding this comment.
I really don't love bringing this in, especially as we barely need a lock. (It's used to manage an environment variable we only want to set when processing custom completions.) There's a standard library atomic-swap-based lock that we could use instead; I'll look at making that change until we can switch to using Synchronization.Mutex.
|
superseded by: #804 |
3 participants
This is a new attempt similar to #674 to only use
FoundationEssentialsif it is available. It should address #748.To enable the usage of only
FoundationEssentialssome API usage was replaced with either API from the StdLib (eg.replaceOccurances->replace).Instead of using
range(_)method, we use the_range(_)method pulled fromswift-foundationitself.However, to make
Mutex<T>work withoutNSLock, I pulled inNIOLockfromswift-nio. Unfortunately, I did not find a nice way to make it work withSynchronization.Mutexinstead.Tests pass on swift 5.7 to 6.0 on linux.