-
Notifications
You must be signed in to change notification settings - Fork 79
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
Address a number of warnings on Windows #184
base: main
Are you sure you want to change the base?
Conversation
This silences a warning from the newer Swift compiler by re-iterating the `Sendable` conformance.
Silence some warnings from the compiler about the unnecessary `#require` which were being used for the unwrapping.
`unlink` is deprecated on Windows, and `RemoveFileW` should be preferred. However, that would limit the path to `MAX_PATH` (261) characters. Prefer to use `FileManager to remove the file to avoid the path limit.
Windows marks `strerror` as deprecated. `strerror_r` is the preferred spelling on POSIX systems, `strerror_s` is preferred on Windows. There is a limit of 94 characters on the error description.
Remove the unnecessary `#expect` wrapping expressions. This was causing warnings due to the comparison of a non-nil value.
The deserialization is unused, assign the value to the blackhole to avoid a warning.
`strdup` is marked as deprecated, preferring `_strdup`. The wrapper avoids the deprecation warning.
@swift-ci please test |
@@ -324,7 +324,7 @@ public final class VariantGroup: GroupTreeReference, BuildFileRepresentable, @un | |||
|
|||
|
|||
/// A ProductReference represents the product of a StandardTarget object. It acts as a placeholder so that product can be represented in other targets, but it contains no meaningful information itself; rather, it vends information about itself by querying its target for that information. A ProductReference object is not part of a product's group tree and has no parent property; rather, it is owned by and has an unowned backpointer to its target. | |||
public final class ProductReference: Reference, BuildFileRepresentable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't safe yet, I need to refactor how it initializes the public unowned var target: Target?
backreference before it's safe to apply @unchecked Sendable
@@ -683,14 +683,14 @@ private final class ProjectModelItemClass: ProjectModelItem { | |||
#expect(fileGroup.children.count == 2) | |||
|
|||
// Examine its children | |||
if let fileRef = try? #require(fileGroup.children.first as? FileReference) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We keep having to flip these #requires back and forth due to compiler bugs, I don't know that there's a way to avoid the warnings on every platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a platform thing or a compiler version thing? I'm building with a relatively recent compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it it's a compiler version thing: swiftlang/swift#79202
@@ -12,6 +12,12 @@ | |||
|
|||
import Foundation | |||
|
|||
#if os(Windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This brings back memories :)
@@ -562,9 +562,13 @@ class LocalFS: FSProxy, @unchecked Sendable { | |||
} | |||
|
|||
func remove(_ path: Path) throws { | |||
#if os(Windows) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know that there's any value in this being platform-conditional. I think this is only using unlink in the first place out of some early attempt to avoid Foundation for whatever reason at the time which is long since irrelevant. And we're using FileManager in the implementation of most of these FSProxy these functions anyways, so we may as well get to benefit from Foundation's work.
public import Foundation | ||
|
||
private func strerror(_ code: CInt) -> String { | ||
withUnsafeTemporaryAllocation(of: CChar.self, capacity: 95) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
95?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
94 + 1; the strerror message is limited to 94 characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, could you encode that in a comment (with a link to docs?) so it doesn't look like an arbitrary magic to future readers?
This set cleans up most of the non-Concurrency related warnings on Windows.