Android: add better nullability checks for nullability annotations added in NDK 26 #444
Conversation
finagolfin
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
| @@ -790,7 +790,11 @@ public final class LocalFileOutputByteStream: FileOutputByteStream { | |||
| override final func writeImpl(_ bytes: ArraySlice<UInt8>) { | |||
| bytes.withUnsafeBytes { bytesPtr in | |||
| while true { | |||
| #if os(Android) | |||
| let n = fwrite(bytesPtr.baseAddress!, 1, bytesPtr.count, filePointer) | |||
There was a problem hiding this comment.
This force-unwrap works fine on linux too, but I separated it out anyway in case there is concern it breaks other platforms.
There was a problem hiding this comment.
I see this done elsewhere in this repo so should be fine, doing the force unwrap on all platforms now.
There was a problem hiding this comment.
ArraySlice currently never passes a buffer with a nil baseAddress. It's arguably an implementation detail, but we rely on it in many places.
There was a problem hiding this comment.
OK, I only modified this because the compiler complained and errored. Is there something you'd like me to change here?
There was a problem hiding this comment.
No, I just wanted to further clarify why the force unwrap is correct here.
| @@ -24,7 +24,7 @@ public final class PseudoTerminal { | |||
| if openpty(&primary, &secondary, nil, nil, nil) != 0 { | |||
| return nil | |||
| } | |||
| guard let outStream = try? LocalFileOutputByteStream(filePointer: fdopen(secondary, "w"), closeOnDeinit: false) else { | |||
| guard let outStream = try? LocalFileOutputByteStream(filePointer: fdopen(secondary, "w")!, closeOnDeinit: false) else { | |||
There was a problem hiding this comment.
I didn't bother separating this out into a guard let since it's just for the tests, can do so if wanted.
Sources/TSCBasic/FileSystem.swift
Outdated
| let fpo = fopen(path.pathString, "rb") | ||
| guard let fp = fpo else { |
There was a problem hiding this comment.
Why not merge these two lines?
| let fpo = fopen(path.pathString, "rb") | |
| guard let fp = fpo else { | |
| guard let fp = fopen(path.pathString, "rb") else { |
There was a problem hiding this comment.
Good point, will change.
Sources/TSCBasic/FileSystem.swift
Outdated
| let fpo = fopen(path.pathString, "wb") | ||
| guard let fp = fpo else { |
There was a problem hiding this comment.
| let fpo = fopen(path.pathString, "wb") | |
| guard let fp = fpo else { | |
| guard let fp = fopen(path.pathString, "wb") else { |
Sources/TSCBasic/Process.swift
Outdated
| @@ -147,6 +147,9 @@ public final class Process { | |||
|
|
|||
| /// The current OS does not support the workingDirectory API. | |||
| case workingDirectoryNotSupported | |||
|
|
|||
| /// The stdin could not be opened. | |||
| case stdinNotOpening | |||
There was a problem hiding this comment.
What about stdinUnavailable? "not opening" is not particularly descriptive, and makes you question what happened.
…ded in NDK 26 Also fix one test.
|
Rebased and made the changes asked for, plus fixed an Android test. |
|
@kateinoigakukun, can I get a CI run here? |
|
@swift-ci Please test |
…ded in NDK 26 (swiftlang#444) Also fix one test.
This is needed because Bionic recently added a bunch of these annotations. I made sure this pull doesn't break anything by testing it on linux x86_64 and with the previous NDK 25c also. I used this patch with others to build the Swift toolchain for my Android CI, finagolfin/swift-android-sdk#122, and the Termux app for Android, which now uses NDK 26b.