-
Notifications
You must be signed in to change notification settings - Fork 441
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
Add ends(with:)
#224
base: main
Are you sure you want to change the base?
Add ends(with:)
#224
Conversation
Sources/Algorithms/EndsWith.swift
Outdated
var possibleSuffixIterator = possibleSuffix.reversed().makeIterator() | ||
for e0 in self.reversed() { | ||
if let e1 = possibleSuffixIterator.next() { |
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 there an efficient way to implement this without needing backwards iteration of the possibleSuffix
?
The only way I can think of is basically a substring search, and checking if it ends up match against the very end.
If such an algorithm existed, it would allow us to loosen the requirement of possibleSuffix
from BidirectionalCollection
down to just Sequence
.
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.
If we loosen the requirement to just Collection
(so that we have a known count
), then we can probably iterate from the middle (starting at self.index(self.endIndex, offsetBy: possibleSuffix.count
), and matching until the end. I'm not sure if this is better though, because we need to do this (potentially-non-constant-time) index math.
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.
Right … I don't imagine that there are many cases where you have a bidirectional collection but you can't make the suffix you're searching for bidirectional.
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 agree.
For completeness, I'll mention an alternative implementation I toyed with, which has different constraints:
self
needs to beRandomAccessCollection
instead ofBidirectional
(and it does some indexing offsetting, which can be slow if that's notO(1)
).- The
possiblePrefix
only needs to be aCollection
instead of aBidirectionalCollection
extension RandomAccessCollection where Element: Equatable {
@inlinable
public func ends<PossibleSuffix: Collection>(
with possibleSuffix: PossibleSuffix,
by areEquivalent: (Element, PossibleSuffix.Element) throws -> Bool
) rethrows -> Bool {
guard let startOfSuffix = index(endIndex, offsetBy: -possibleSuffix.count, limitedBy: startIndex) else {
return false
}
return try self[startOfSuffix...].elementsEqual(possibleSuffix, by: areEquivalent)
}
}
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.
Just a small addition here is that in this case possibleSuffix
probably will be better if constrained to RandomAccessCollection
as well, otherwise -possibleSuffix.count
is not guaranteed to be O(1).
Guides/EndsWith.md
Outdated
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.
One of the pages of the "Guides" should link to this, but I'm not sure which.
I don't see any predicate methods like this elsewhere in the package. Perhaps we should start a new Guides/Predicates.md
category?
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.
any predicate methods like this
Can you say more about what you mean here?
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 was thinking that perhaps this can be the first of several methods that fall under a new "predicates" category, which would contain boolean-returning methods which check for particular qualities in a collection.
One other predicate I've been thinking of is something like someArray.allElementsAreEqual()
, which e.g. would be true for [1, 1]
but not [1, 2]
.
If we find other predicates to add to swift-algorithms
, we can group ends(with:)
among them.
Until then, I'm not sure which "guide" we should document this in.
1163301
to
8524728
Compare
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, @amomchilov! Just a few small notes here.
Sources/Algorithms/EndsWith.swift
Outdated
/// collection are equivalent to the elements in another collection, using | ||
/// the given predicate as the equivalence test. | ||
/// | ||
/// The predicate must be a *equivalence relation* over the elements. That |
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.
Does this typo show up elsewhere in the project?
/// The predicate must be a *equivalence relation* over the elements. That | |
/// The predicate must be an *equivalence relation* over the elements. That |
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 from swift-algorithms
, but in the standard library itself.
I opened a fix: https://github.com/apple/swift/pull/71871/files
/// let a = 8...10 | ||
/// let b = 1...10 | ||
/// | ||
/// print(b.ends(with: a)) | ||
/// // Prints "true" |
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.
Could we use a string-based example here, instead of a range?
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.
@natecook1000 I copied this doc from starts(with:)
and modified it. I think if we change this, we should starts(with:)
to match.
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'm good with either Arrays or Strings. Perhaps ranges are a bad example, given that some readers might confuse ..<
and ...
. We can pick a more self-evident example that doesn't need to require knowledge of that distinction.
8524728
to
375eb1f
Compare
/// let a = 8...10 | ||
/// let b = 1...10 | ||
/// | ||
/// print(b.ends(with: a)) | ||
/// // Prints "true" |
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.
@natecook1000 I copied this doc from starts(with:)
and modified it. I think if we change this, we should starts(with:)
to match.
/// collection and the length of `possibleSuffix`. | ||
// Adapted from https://github.com/apple/swift/blob/d6f9401/stdlib/public/core/SequenceAlgorithms.swift#L281-L286 | ||
@inlinable | ||
public func ends<PossibleSuffix: BidirectionalCollection>( |
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.
/// collection and the length of `possibleSuffix`. | ||
// Adapted from https://github.com/apple/swift/blob/d6f9401/stdlib/public/core/SequenceAlgorithms.swift#L235-L252 | ||
@inlinable | ||
public func ends<PossibleSuffix: BidirectionalCollection>( |
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.
Sources/Algorithms/EndsWith.swift
Outdated
/// collection are equivalent to the elements in another collection, using | ||
/// the given predicate as the equivalence test. | ||
/// | ||
/// The predicate must be a *equivalence relation* over the elements. That |
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 from swift-algorithms
, but in the standard library itself.
I opened a fix: https://github.com/apple/swift/pull/71871/files
375eb1f
to
17dfa21
Compare
@inlinable | ||
public func ends<PossibleSuffix: BidirectionalCollection>( | ||
with possibleSuffix: PossibleSuffix, | ||
by areEquivalent: (Element, PossibleSuffix.Element) throws -> Bool |
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.
Since equivalence is dictated by areEquivalent
does this need to be constrained to element is Equatable
(BidirectionalCollection where Element: Equatable
)?
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.
Nice catch! I moved this overload into its own unconstrained extension.
@natecook1000 Is this looking good to merge? |
This PR adds an end-counterpart to the built-in
Sequence.starts(with:)
.Checklist