add suffix:while method

[michiboo]

Thanks for contributing to Swift Algorithms!



- [x] I've completed this task
- [ ] This task isn't completed

Add a suffix(while:) method #62

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

[mdznr]

Excited to see this function added, bringing more parity between the suffix and prefix family of functions!

If changes are still being worked on, I’d recommend making use of a new feature on GitHub: Drafts. By marking this PR as a draft, it helps to convey the author’s intended state of the changes.^[1]

I left a few small comments inline, but want to emphasize the primary high-level piece of feedback here, which may make several of the comments inapplicable. I think suffix(while:) should return SubSequence instead of [Element] for consistency and performance reasons.

---

1. https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/changing-the-stage-of-a-pull-request

[mdznr]

Suggested change

	public __consuming func Suffix(while predicate: (Element) throws -> Bool
	public __consuming func suffix(
	while predicate: (Element) throws -> Bool

[mdznr]

Can this return SubSequence instead?

prefix(while:) on Collection returns a SubSequence.^[1]

---

1. https://github.com/apple/swift/blob/main/stdlib/public/core/Collection.swift#L1328

[mdznr]

Suggested change

	// Suffix(while:)
	// suffix(while:)

[mdznr]

Documentation should be added. Mirroring the documentation for prefix(while:) would be a good start.

[mdznr]

This double-reverse operation and converting to Array shouldn’t be necessary. The last index of the collection can be retrieved with endIndex, then walked backwards using index(before:).

This would be similar to the implementation of prefix(while:)^[1], but a little bit trickier. Since endIndex is beyond the subscript-able range of a collection, the index has to be moved backwards before using it to subscript the collection, and ensuring it doesn’t go before the first element (start index). In SuffixTests.swift, I made some comments about test cases that will be important for ensuring that these cases are accounted for.

---

1. https://github.com/apple/swift/blob/main/stdlib/public/core/Collection.swift#L1330

[mdznr]

Suggested change

	// Copyright (c) 2020 Apple Inc. and the Swift project authors
	// Copyright (c) 2021 Apple Inc. and the Swift project authors

[mdznr]

Suggested change

	// Copyright (c) 2020 Apple Inc. and the Swift project authors
	// Copyright (c) 2021 Apple Inc. and the Swift project authors

[mdznr]

Consistent indentation

Suggested change

	var result = ContiguousArray<Element>()
	self.reverse()
	var result = ContiguousArray<Element>()
	self.reverse()

[mdznr]

I don’t believe reverse() can be used here (won’t compile), because it is a mutating function that reverses the elements of the collection in-place. This function, suffix(while:), isn’t mutating (and should remain that way), so it can’t use self.reverse() in its implementation.

It seems that you might have meant to use the non-mutating version of that function, reversed(), which returns a reversed collection:

    for element in self.reversed() {

However, this comment likely won’t be applicable when changing the function to return SubSequence instead.

[CTMacUser]

What about something like (neither compiled nor tested):

extension BidirectionalCollection {
  public func suffix(
    while predicate: (Element) throws -> Bool
  ) rethrows -> SubSequence {
    let start = startIndex
    var result = endIndex
    while result > start {
      let previous = index(before: result)
      guard try predicate(self[previous]) else { break }

      result = previous
    }
    return self[result...]
  }
}

?

[mdznr]

@CTMacUser I think that’s pretty much exactly what I imagined the implementation would look like.

Although, I think we might want to change

while result > start {

to

while result != start {

A lot of similar algorithms in the standard library use != instead of > or < for comparing indexes. I’ll have to do more research as to why that is. My initial guess is perhaps because implementations of custom indexes have slightly faster implementations of != than <. But either way, it would be good to have consistency here.

[kylemacomber]

A lot of similar algorithms in the standard library use != instead of > or < for comparing indexes. I’ll have to do more research as to why that is. My initial guess is perhaps because implementations of custom indexes have slightly faster implementations of != than <. But either way, it would be good to have consistency here.

I agree that we should use != to be consistent with the stdlib.

One of the reasons stdlib uses != is historical. Originally Collection.Index in the stdlib was only Equatable, not Comparable. From SE-0065 which added the Comparable requirement:

It's worth noting that this requirement isn't strictly necessary. Without it, though, indices would have no requirements beyond Equatable, and creation of a Range<T> would have to be allowed for any T conforming to Equatable. As a consequence, most interesting range operations, such as containment checks, would be unavailable unless T were also Comparable, and we'd be unable to provide bounds-checking in the general case.

That said, the requirement has real benefits. For example, it allows us to support distance measurement between arbitrary indices, even in collections without random access traversal. In the old model, x.distance(to: y) for these collections had the undetectable precondition that x precede y, with unpredictable consequences for violation in the general case.

[natecook1000]

Thanks for your contribution, @michiboo! Could you update your tests to compile and pass? It looks like this is otherwise ready to go.

[natecook1000]

This is all set now — thanks for this implementation, @michiboo!