Stdlib serialization redux #1205
Conversation
| { | ||
| let sourceDirectory = URL(fileURLWithPath: #filePath) | ||
| .deletingLastPathComponent().deletingLastPathComponent().deletingLastPathComponent() | ||
| / "StandardLibrary" / "Sources" |
There was a problem hiding this comment.
FWIW, I loathe operators for pretty much anything other than arithmetic operations on numerics. It is also not at all obvious to me that / has to be understood as a path separator when its operands are arbitrary expressions.
|
The problem causing the "unexpected |
| .compactMap { $0 as? URL } | ||
| .filter { $0.pathExtension == "hylo" } | ||
|
|
||
| let freestrandingDirectory = sourceDirectory / "Core" | ||
| let freestandingSources = hostedSources.filter(freestrandingDirectory.contains) |
There was a problem hiding this comment.
So we're going over these files 3 times! I think we're starting to have enough sources in the standard library for that to become noticeably inefficient.
There was a problem hiding this comment.
There are:
.compactMap { $0 as? URL }.filter { $0.pathExtension == "hylo" }.filter(freestrandingDirectory.contains)
We should merge the two first ones in a single compactMap. Then maybe we might also want to declare hostedSources and freestandingSources as lazy collections.
There was a problem hiding this comment.
There is no way this code will be performance critical, and I thought to prioritize readability. Do you really think those changes would improve it?
There was a problem hiding this comment.
I am not as confident as you that there can't be any performance impact here here but regardless I think the changes would improve readability. This kind of code will invariably make me pause and look for a reason why the filters weren't expressed that way. At the very least I think there's a missing abstraction here.
There was a problem hiding this comment.
performance impact
Seriously, in a build tool plugin? The cost of reading directory contents (to say nothing of reading actual files off disk) would surely swamp these costs.
I think the changes would improve readability.
Then you should suggest the change exactly as you would like to see it written, and I will accept it without argument. I would not be able to judge any such change to be a readability improvement, and I don't want to guess at what will satisfy you.
The addition of .lazy at the beginning would not hurt readability too much but you have often found that to be a performance cost. If lazy is effective, it's likely in principle that the other changes couldn't have any effect on performance in a release build (in some world where the performance of this code actually matters) because the lazy layers collapse. So I don't see the point at all, but if you want it changed, just say how and I will do it.
| .filter { $0.pathExtension == "hylo" } | ||
|
|
||
| let freestrandingDirectory = sourceDirectory / "Core" | ||
| let freestandingSources = hostedSources.filter(freestrandingDirectory.contains) |
There was a problem hiding this comment.
Thar's a algorithm for that.
| let freestandingSources = hostedSources.filter(freestrandingDirectory.contains) | |
| let freestandingSources = hostedSources.filter { $0.pathComponents.starts(with: freestrandingDirectory.pathComponents) } |
I didn't put it in contains because that function doesn't do what it says on the tin if you read the comment carefully.
There was a problem hiding this comment.
Sorry, which function doesn't do what it says it does, and on what?
There was a problem hiding this comment.
URL.contains, whose documentation says “returns true if self is the path of a directory containing other.”. In fact it doesn't tell you anything whether self refers to a directory (which requires verifying that it's a file:// URL and then looking at disk) or whether it contains other. in fact it only tells you whether the path of self is a parent of the path of other. Potential for little inaccuracies (or at the very least, ambiguities) like that one, is among the many problems with URL that make me want to write a first-class path abstraction.
dabrahams
force-pushed
the
stdlib-serialization-redux
branch
from
40aa49d to
7036839
Compare
Apply swift-format Attempt to actually serialize Fix the gathering of standard library sources Fix the loading of serialized resources Swift format
dabrahams
force-pushed
the
stdlib-serialization-redux
branch
from
7036839 to
92df416
Compare
|
@kyouko-taiga now that we're not really dependent on SPM anymore we should discuss whether this PR is going in the right direction and whether I should try to land it or close it. |
|
Sorry for the radio silence. I will look into it after my vacation as part of my effort to bring modules. |
Currently blocked on swiftlang/swift#70979