FilePath syntactic operations #14
Conversation
milseman
requested review from
lorentey,
compnerd,
kylemacomber and
abertelrud
milseman
force-pushed
the
route_morphology
branch
from
10a026e to
0c8a236
Compare
milseman
force-pushed
the
route_morphology
branch
from
0c8a236 to
6cb1275
Compare
|
Overall this looks great! Really excited to see this API! |
Add CInterop namespace and PlatformChar/PlatformEncoding typealiases for narrow/wide characters. Change Windows syscalls to use wide-character variants. Add full Windows root parser to error-correct malformed roots and correctly normalize roots and post-root portions of the path. Add platformString APIs as replacements for cString APIs. Add internal SystemString as backing storage type for FilePath. Add Windows path mocking support. This change includes API/ABI additions and deprecations.
Add the proposed non-file-system-interacting FilePath operations, Component, Root, and ComponentView. Proposal: https://forums.swift.org/t/api-review-filepath-syntactic-apis-version-2/44197 This change includes API/ABI additions and deprecations.
milseman
force-pushed
the
route_morphology
branch
from
11d10eb to
2167575
Compare
This API is fully redundant with .kind == .regular and could be a source of confusion and namespace pollution. Drop it for now, and we can add something equivalent in the future when we have more component-analysis APIs.
milseman
changed the title
|
|
||
| public subscript(position: Index) -> FilePath.Component { | ||
| let end = _path._parseComponent(startingAt: position._storage).componentEnd | ||
| return FilePath.Component(_path, position._storage ..< end) |
There was a problem hiding this comment.
Is it worth making ComponentView.Index store a Range so subscript is O(1)?
There was a problem hiding this comment.
It is amortized constant, as in a subscript over every index will be a total of O(n). We could store the range, but that would complicate startIndex and/or the initializer for ComponentView. If we have an unknown representation for the length, that further complicates other logic. subscript is not the only operation to optimize for, as many operations (especially RRC operations) only advance indices or slice the collection.
This "parse" is just scanning bytes looking for the / byte (or \ on Windows). I don't think it's worth spending the complexity budget on, since these types are resilient, these operations are not inlinable, and the scanned byte range is loaded at the end (with a retain, at least until we do small-form for small components).
There was a problem hiding this comment.
It's good we have the flexibility to add this in the future.
Computing the Range in the ComponentView initializer seems like the right direction to me. It's the direction we've taken with the ChunkedByCount and SlidingWindows collections from the Algorithms package.
There was a problem hiding this comment.
By "resilient" I wasn't referring to the ability to add it later (though that's true), I was referring to the performance characteristics of the code. Inside the call, subscript does something computers are really good at (linear scan within a cache line for data that will be loaded anyways), after having been called indirectly and returning its result indirectly.
From a performance perspective, I don't think ChunkedByCount and SlidingWindows are analogous, as their collection status is primarily for the purpose of yielding elements. They're not RRC, for example.
milseman
force-pushed
the
route_morphology
branch
from
45a3ec4 to
68fe5cc
Compare
| return self[..<idx] | ||
| } | ||
|
|
||
| internal mutating func _eatThrough(_ idx: Index) -> Slice { |
There was a problem hiding this comment.
I wonder if these should be eat(upTo:) and eat(through:), taking their cue from prefix(upTo:) and prefix(through:)?
There was a problem hiding this comment.
They could be here. It depends on what the total overload set is, as argument labels don't play as well with trailing closures. Either way, these are temporary helpers that I can hopefully drop in the future.
| self.formIndex(after: &readIdx) | ||
| } | ||
| } | ||
| self.removeLast(self.distance(from: writeIdx, to: readIdx)) |
There was a problem hiding this comment.
Interesting so this is basically:
uniqueAdjacentDuplicates { isSeparator($0) }Maybe we need to add that algorithm after all 🤔
There was a problem hiding this comment.
Now that we have a prepass to convert / to \ on Windows, it is. It used to also do the separator change here, but that makes root parsing significantly more complex.
|
|
||
| extension SystemString { | ||
| internal init() { | ||
| self.nullTerminatedStorage = [.null] |
There was a problem hiding this comment.
A few times in the code you mention "Intern[ing] the empty SystemString," is this as simple as declaring a global or static variable to store this [.null]?
There was a problem hiding this comment.
It can be, but IIRC how you declare a global matters and we don't want to unnecessarily dirty memory. I never understood all the differences (especially as they interact with @usableFromInline, which wouldn't be relevant here yet).
Fix bug in _modify and add better testing. Change lexicallyNormal to lexicallyNormalized() to match "ed" rule. Fix up some comments and remove some dead code Co-authored-by: Kyle Macomber <[email protected]>
milseman
force-pushed
the
route_morphology
branch
from
d437b38 to
d24083e
Compare
Syntactic operations for FilePath, inspired by Rust and C++17. Includes Windows support.
Review thread