Make `IndexingIterator.position` public

I've got some code that looks like the following:

static func doSomething<InputSlice: Collection>(_ input: InputSlice) -> Bool {

  let firstIndex = input.startIndex
  guard firstIndex != input.endIndex, input[firstIndex] == someValue else {
    return pathOne(input)
  }
  let secondIndex = input.index(after: firstIndex)
  guard secondIndex != input.endIndex, input[secondIndex] == someValue else {
    return pathTwo(input)
  }
  let sectionBegin = input.index(after: secondIndex)
  return pathThree(input[sectionBegin...])
}

Basically: if it can't find a particular leading value (because the collection isn't long enough or contains a different value), it takes pathOne. If that value isn't followed by another particular value, it takes pathTwo. If it finds the expected leading pattern, it chops it off and hands the remainder over to pathThree.

It's not that complicated, but it's also not that easy to read. Ideally, I'd like to express this by using an iterator:

static func doSomething<InputSlice: Collection>(_ input: InputSlice) -> Bool {

  var iterator = IndexingIterator(input)
  guard iterator.next() == someValue else {
    return pathOne(input)
  }
  guard iterator.next() == someValue else {
    return pathTwo(input)
  }
  let sectionBegin = iterator.position // <-
  return pathThree(input[sectionBegin...])
}

That's much easier to understand, right? Unfortunately, IndexingIterator doesn't expose its position property:

So I'd like to propose making this property public. The index is a necessary implementation detail of IndexingIterator, so it seems unlikely that we would ever remove it or change the implementation in such a way that supporting this API becomes a burden.

How about?:

var section = input[...]
guard section.popFirst() == someValue else {
  return pathOne(input)
}
guard section.popFirst() == someValue else {
  return pathTwo(input)
}
return pathThree(section)
3 Likes

That's very nice!

Unfortunately it ends up looking a bit weird to bind the slice to the name section so early, so I may need to give it some generic name like "remainder", but it's definitely much easier to follow.

One of my few rules of thumb when working with collections are

  • Question any use of Iterator outside of the implicit for syntax.
  • Question any use of types not tied to the collection, e.g., Slice<X> instead of X.SubSequence.

So your original code is definitely a red flag for me.

That seems like a preference more than a “red flag”. Iterators are very useful in lots of cases, and there’s nothing incorrect about using their concrete types directly if they duplicate the algorithm you’d otherwise be writing by hand - even if the collection defines a different preferred iterator for “for” loops.

In general, I actually try to avoid working in terms of mutable slices; it feels incomplete due to missing functions like popFirst(_: Int) or pop(while:), and it can easily lead to using convenience functions which introduce unnecessary bounds checks. That’s just my preference, and it’s partly shaped by the particular project I’m working on, where bounds checking can be a significant overhead.

Fair point, I don't mean the red flag as in a potentially incorrect code. More like, you're not fully utilizing the protocol hierarchy.

The Iterator capabilities is strictly superseded by SubSequence, so choosing the former should at least be a precise conscious decision rather than a defaulted ones.

That you (try to) use IndexingIterator instead of Iterator is actually the more offending one. You're assuming that it's the best tool for running through collection, when the collection itself may disagree. If you'd expect that to be the case, it might as well be reflected in the function signature.

It sounds like a premature optimization, but heck, I'm saying this without a cold hard number, or even a binary to compare, so :woman_shrugging:.

Actually, the goal is precisely not to reflect it in the function signature - I don’t need IndexingIterator to be the collection’s preferred iterator, but if it proved its position, I would use it instead of manually advancing indexes.

Even when the collection provides a different iterator, It does not “disagree” or in any way preclude me from using a generic one. That’s what generic code means. So long as it is a conforming collection, I can iterate it via any infrastructure I deem appropriate.

You’re right - you do lack the evidence to make that statement. Luckily we have -Ounchecked, so I know for a fact that I could lower my runtime by up to 25% if I can find a way to safely eliminate bounds and overflow checks.

It's always a juggle between the holy trinity of maintainability, performance, and readability. If you project demands absolute performance, then, you do you, the 25% figure won't matter (and is probably a stretch anyway).

Still, I'm not so sure that all that hard work will improve things appreciably, or even noticeably. Compilers beat me to optimization before (and they really should), so I won't be holding my breath.

Terms of Service

Privacy Policy

Cookie Policy