-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Reduce main thread usage in SafeDITool #114
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #114 +/- ##
==========================================
+ Coverage 99.52% 99.59% +0.07%
==========================================
Files 33 33
Lines 2724 2728 +4
==========================================
+ Hits 2711 2717 +6
+ Misses 13 11 -2
|
@@ -523,8 +523,7 @@ extension TypeSpecifierListSyntax { | |||
if case let .simpleTypeSpecifier(simpleTypeSpecifierSyntax) = specifier { | |||
simpleTypeSpecifierSyntax.specifier.text | |||
} else { | |||
// lifetimeTypeSpecifier is SPI, so we ignore it. | |||
nil | |||
nil // lifetimeTypeSpecifier is SPI, so we ignore it. |
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.
unrelated change, but it helps up our code coverage so ¯\(ツ)/¯
let includedFileEnumerator = fileFinder | ||
.enumerator( |
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.
eek. we were previously enumerating directories on disk on main.
@@ -314,5 +327,9 @@ protocol FileFinder { | |||
} | |||
|
|||
extension FileManager: FileFinder {} | |||
extension FileManager: @retroactive @unchecked Sendable { | |||
// FileManager is thread safe: |
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.
the delegate methods aren't thread safe. but we aren't using those
@@ -1733,6 +1733,7 @@ final class SafeDIToolCodeGenerationErrorTests: XCTestCase { | |||
tool.moduleInfoOutput = nil | |||
tool.dependentModuleInfoFilePath = nil | |||
tool.dependencyTreeOutput = nil | |||
tool.dotFileOutput = nil |
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.
not strictly speaking necessary but it is best to set every property on the tool before trying to run it to avoid spurious errors.
### Requirements | ||
|
||
* Xcode 16.0 or later | ||
* iOS 13 or later | ||
* tvOS 13 or later | ||
* watchOS 6 or later | ||
* macOS 10.13 or later |
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.
deferring instead to Package.swift
. Requirements are effectively "you can compile the Swift 6 toolchain and are compiling on a target that supports SwiftSyntax"
public func removingEmpty() -> [Element] { | ||
filter { !$0.isEmpty } | ||
} |
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 were doing this in enough places, it felt like a reasonable addition.
No description provided.