AttributedString Index Validity APIs #1177
Conversation
jmschonfeld
requested review from
parkera,
lorentey,
iCharlesHu and
itingliu
|
@swift-ci please test |
jmschonfeld
force-pushed
the
attrstr/index-validation
branch
from
98cabe4 to
877b55f
Compare
|
@swift-ci please test |
|
@swift-ci please test macOS platform |
| self.lowerBound >= text.startIndex && | ||
| self.lowerBound <= text.endIndex && | ||
| self.upperBound._version == text.__guts.version && | ||
| self.upperBound >= text.startIndex && |
There was a problem hiding this comment.
Nit: Checking my logic here... Do you need both self.lowerBound >= text.startIndex and self.upperBound >= text.startIndex?
I thought a Range has the precondition of lowerBound<=upperBound, so doesn't self.lowerBound >= text.startIndex guarantee self.upperBound >= text.startIndex?
Same question goes with self.lowerBound <= text.endIndex && self.upperBound <= text.endIndex
There was a problem hiding this comment.
You're right Range has that precondition so both checks aren't required. I can update this to simplify the expressions down a bit
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #if canImport(Synchronization) |
There was a problem hiding this comment.
In other places we have #if canImport(Synchronization) && FOUNDATION_FRAMEWORK. Is that wrong?
There was a problem hiding this comment.
Previously we needed the && FOUNDATION_FRAMEWORK because non-FOUNDATION_FRAMEWORK builds ran on a very old version of macOS in CI where we back deployed back to a version that didn't have Synchronization (even though the canImport(Synchronization) check passes because it was in the SDK). Now that we've bumped the deployment target of the package, we can safely use the Synchronization module on the macOS version CI is back deployed to, so no need to guard with FOUNDATION_FRAMEWORK (but there are still some edge case situations where the module doesn't exist at all that we need to guard for with the canImport check)
| private static let _nextVersion = Atomic<Version>(0) | ||
|
|
||
| static func createNewVersion() -> Version { | ||
| _nextVersion.wrappingAdd(1, ordering: .relaxed).oldValue |
There was a problem hiding this comment.
Nit: I'd probably put the #if canImport(Synchronization) inside this function since that's implementation detail
There was a problem hiding this comment.
Sure I can do that if you prefer - I still would need a separate canImport check around the private static let variables since their types are different, but if you want I can split it into two canImport checks, one for around the variable and one within the body of the function
|
@swift-ci please test |
|
@swift-ci please test |
This PR implements the index validity checking APIs as proposed in SF-0015 by adding a new version number tracked within each
AttributedStringandAttributedString.Index. As neither type is frozen, we have the flexibility in the future to optimize this (for example, by taking advantage of the version number stored within the already-nestedBigString.Indexif the swift-collections package provided access to it) if we deem necessary.