Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add ends(with:) #224

wants to merge 1 commit into from

Conversation

amomchilov
Copy link
Contributor

@amomchilov amomchilov commented Feb 18, 2024

This PR adds an end-counterpart to the built-in Sequence.starts(with:).

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

Comment on lines 81 to 83
var possibleSuffixIterator = possibleSuffix.reversed().makeIterator()
for e0 in self.reversed() {
if let e1 = possibleSuffixIterator.next() {
Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@amomchilov amomchilov Feb 25, 2024

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:

  1. self needs to be RandomAccessCollection instead of Bidirectional (and it does some indexing offsetting, which can be slow if that's not O(1)).
  2. The possiblePrefix only needs to be a Collection instead of a BidirectionalCollection
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)
  }
}

Copy link
Contributor

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).

Copy link
Contributor Author

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?

Copy link
Member

@natecook1000 natecook1000 Feb 20, 2024

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?

Copy link
Contributor Author

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.

Sources/Algorithms/EndsWith.swift Outdated Show resolved Hide resolved
Copy link
Member

@natecook1000 natecook1000 left a 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 Show resolved Hide resolved
Sources/Algorithms/EndsWith.swift Outdated Show resolved Hide resolved
/// 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
Copy link
Member

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?

Suggested change
/// The predicate must be a *equivalence relation* over the elements. That
/// The predicate must be an *equivalence relation* over the elements. That

Copy link
Contributor Author

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

Comment on lines +25 to +29
/// let a = 8...10
/// let b = 1...10
///
/// print(b.ends(with: a))
/// // Prints "true"
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Comment on lines +25 to +29
/// let a = 8...10
/// let b = 1...10
///
/// print(b.ends(with: a))
/// // Prints "true"
Copy link
Contributor Author

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>(
Copy link
Contributor Author

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>(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// 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
Copy link
Contributor Author

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

@inlinable
public func ends<PossibleSuffix: BidirectionalCollection>(
with possibleSuffix: PossibleSuffix,
by areEquivalent: (Element, PossibleSuffix.Element) throws -> Bool
Copy link
Contributor

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amomchilov
Copy link
Contributor Author

@natecook1000 Is this looking good to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants