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 commonPrefix(with:), commonSuffix(with:) #153

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

timvermeulen
Copy link
Contributor

@timvermeulen timvermeulen commented Jul 23, 2021

Find the common prefix/suffix of two sequences or collections.

  • commonPrefix(with:)
  • commonSuffix(with:)
  • endOfCommonPrefix(with:): get a pair of indices that mark the end of the common prefix of two collections
  • startOfCommonSuffix(with:): get a pair of indices that mark the start of the common suffix of two collections
extension Sequence {
  public func commonPrefix<Other: Sequence>(
    with other: Other,
    by areEquivalent: (Element, Other.Element) throws -> Bool
  ) rethrows -> [Element]
}

extension Sequence where Element: Equatable {
  public func commonPrefix<Other: Sequence>(
    with other: Other
  ) -> CommonPrefix<Self, Other> where Other.Element == Element
}

extension LazySequenceProtocol {
  public func commonPrefix<Other: Sequence>(
    with other: Other,
    by areEquivalent: @escaping (Element, Other.Element) -> Bool
  ) -> CommonPrefix<Self, Other>
}

extension Collection {
  public func commonPrefix<Other: Sequence>(
    with other: Other,
    by areEquivalent: (Element, Other.Element) throws -> Bool
  ) rethrows -> SubSequence
}

extension Collection where Element: Equatable {
  public func commonPrefix<Other: Sequence>(
    with other: Other
  ) -> SubSequence where Other.Element == Element
}

extension LazyCollectionProtocol where Element: Equatable {
  public func commonPrefix<Other: Sequence>(
    with other: Other
  ) -> CommonPrefix<Self, Other> where Other.Element == Element
}

extension BidirectionalCollection {
  public func commonSuffix<Other: BidirectionalCollection>(
    with other: Other,
    by areEquivalent: (Element, Other.Element) throws -> Bool
  ) rethrows -> SubSequence
}

extension BidirectionalCollection where Element: Equatable {
  public func commonSuffix<Other: BidirectionalCollection>(
    with other: Other
  ) -> SubSequence where Other.Element == Element
}

extension Collection {
  public func endOfCommonPrefix<Other: Collection>(
    with other: Other,
    by areEquivalent: (Element, Other.Element) throws -> Bool
  ) rethrows -> (Index, Other.Index)
}

extension Collection where Element: Equatable {
  public func endOfCommonPrefix<Other: Collection>(
    with other: Other
  ) -> (Index, Other.Index) where Other.Element == Element
}

extension BidirectionalCollection {
  public func startOfCommonSuffix<Other: BidirectionalCollection>(
    with other: Other,
    by areEquivalent: (Element, Other.Element) throws -> Bool
  ) rethrows -> (Index, Other.Index)
}

extension BidirectionalCollection where Element: Equatable {
  public func startOfCommonSuffix<Other: BidirectionalCollection>(
    with other: Other
  ) -> (Index, Other.Index) where Other.Element == Element
}

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

@kylemacomber
Copy link
Contributor

I think it's more natural to not pluralize of start and end, i.e. just have endOfCommonPrefix, startOfCommonSuffix

@timvermeulen
Copy link
Contributor Author

timvermeulen commented Jul 23, 2021

I think it's more natural to not pluralize of start and end, i.e. just have endOfCommonPrefix, startOfCommonSuffix

Fair point. I think I went with this because we already have endOfPrefix(while:) and startOfSuffix(while:) which both return a single index, but that's probably fine.

} else {
return nil
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the iterator needs to add a done flag to its state. For instance, if we do "1234A56" vs. "1234B56", and we iterate past the first nil, we'll get a "5", instead of nil forevermore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you're absolutely right!

by areEquivalent: @escaping (Element, Other.Element) -> Bool
) -> CommonPrefix<Self, Other> {
CommonPrefix(base: self, other: other, areEquivalent: areEquivalent)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these be using Elements and elements instead of Self and self?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a way to do it, but this is intentional. If you wrap elements instead of self then the type no longer contains any sign of laziness, so you'll need to wrap it in LazySequence<...> again to add the laziness back. But by wrapping self, all we need is for CommonPrefix to conditionally conform to LazySequenceProtocol when the (first) base collection does, in order to propagate the laziness.

CommonPrefix also can't unconditionally conform to LazySequenceProtocol because it's also returned by the Sequence overloads, so in a non-lazy context.

by areEquivalent: @escaping (Element, Other.Element) -> Bool
) -> CommonPrefix<Self, Other> {
CommonPrefix(base: self, other: other, areEquivalent: areEquivalent)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in the LazySequenceProtocol section: shouldn't these be using Elements and elements instead of Self and self?

@CTMacUser
Copy link
Contributor

Don't endsOfCommonPrefix(with:) and startssOfCommonSuffix(with:) overlap with diverges(from:) and converges(with:) from #37 ?

@timvermeulen timvermeulen marked this pull request as ready for review August 5, 2021 16:32
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.

None yet

3 participants