fix build on Xcode 13 RC #78
Conversation
`strdup` returns an implicitly unwrapped optional, which seems to loose its "implicit" via the assignment. I couldn't find anything in the release docs, but I guess it's a fair change :-) Let's use them as proper optionals and instead of crashing, return defaults.
There was a problem hiding this comment.
The reasons that strdup will return NULL are when insufficient memory is available.
RETURN VALUE
On success, the strdup() function returns a pointer to the duplicated string. It returns
NULL if insufficient memory was available, with errno set to indicate the error.
I would imagine that in the past Swift would have caused a crash in this case, the change to the compiler exposes strdups error case to to caller. I'd suggest we retain the existing behaviour because silently handling errors and returning false information is unexpected behaviour.
| @@ -587,8 +587,8 @@ extension Path { | |||
|
|
|||
| extension Path { | |||
| public static func glob(_ pattern: String) -> [Path] { | |||
| guard let cPattern = strdup(pattern) else { return [] } | |||
There was a problem hiding this comment.
Returning an empty array here is perhaps undesired as from the callers perspective, they will believe that the glob operation showed there have been no matched files. Which is not necessarily the case, we instead failed to load them.
| @@ -619,8 +619,7 @@ extension Path { | |||
| } | |||
|
|
|||
| public func match(_ pattern: String) -> Bool { | |||
| let cPattern = strdup(pattern) | |||
| let cPath = strdup(path) | |||
| guard let cPattern = strdup(pattern), let cPath = strdup(path) else { return false } | |||
There was a problem hiding this comment.
Returning false here represents to the caller that the pattern did not match, which may not be true.
Right right, the return value has always been an implicitly unwrapped optional. [snip]
Did a bit more reading, and it's actually one of the examples from https://swift.org/blog/iuo/ So either a bug was fixed that this now works as expected, or
Fair, thanks for the feedback. Happy to put up a change! It feels like a |
|
I dont know if this is worse but in my PR I just safely unwrapped them to free them, given that if they come back as nil, they probably were already freed? |
My hunch is it would crash a few lines down. So my hunch is it would just crash there then :-/ |
|
It seems like we're blocked here but it's not clear to me what the next steps are. |
|
@diederich what about somthing like this: guard let ... = strdup(...) else {
fatalError("strdup returned null: Likely out of memory")
}Potentially message can contain string interpolation for the errno if that's exposed. |
|
Implemented some of that feedback over here: #80 |
|
#80 LGTM! Thanks @chrisballinger 🙏 |
This fixes the build of PathKit on Xcode 13 RC.
strdupreturns an implicitly unwrapped optional, which seems to loose its "implicit" via the assignment.I couldn't find anything in the Xcode release notes, but I guess it's a fair change :-)
Let's treat the returned pointers as true optionals and instead of crashing, return defaults.