Improve traverseViaChain API #3535
Conversation
|
@travisbrown I know I'm making your life a pain here with these PRs. I'm sorry. I do hope we can get this in and not have mima force us to keep an unneeded mutable API. |
|
No, this is great. Happy to review in the morning. |
Codecov Report
@@ Coverage Diff @@
## master #3535 +/- ##
==========================================
+ Coverage 91.34% 91.41% +0.07%
==========================================
Files 386 386
Lines 8592 8540 -52
Branches 241 241
==========================================
- Hits 7848 7807 -41
+ Misses 744 733 -11 |
|
Okay, the 2.13 build brought up an interesting issue: in 2.12, This is probably a minor performance hit, but I imagine very very minimal (since the real meat of what traverse is doing is still the Eval and the applicative context I assume. I think I'm happy with this now if you can take a look @travisbrown |
|
@travisbrown what do you think about this one? |
| def wrapMutableIndexedSeq[A](m: mutable.IndexedSeq[A]): ImIndexedSeq[A] = | ||
| new WrappedIndexedSeq(m) | ||
|
|
||
| private[kernel] class WrappedIndexedSeq[A](m: mutable.IndexedSeq[A]) extends ImIndexedSeq[A] { |
There was a problem hiding this comment.
I can never remember exactly how this works inside objects, but is there any reason to make this package-private (which is just public in the JVM API) instead of just private (which I think should be properly JVM-private)?
There was a problem hiding this comment.
not that I know of, I just copied the idiom below. Should I fix that up?
| @@ -508,11 +500,11 @@ sealed abstract class Chain[+A] { | |||
| val builder = new StringBuilder("Chain(") | |||
| var first = true | |||
|
|
|||
| foreach { a => | |||
| foreachUntil { a => | |||
There was a problem hiding this comment.
This is just miscellaneous clean-up, right, since foreach isn't used anywhere else?
There was a problem hiding this comment.
yes. foreach is used exactly once and is private. Just use foreachUntil (or iterator).
follow up to #3521
In that PR, I added an API that consumes an Iterator. That is a bummer to have an API with a mutable arg, even if most people know that iterators must be considered with ownership semantics.
In fact, the first thing we do is materialize an IndexedSeq, so, why not just take an IndexedSeq and have callers materialize one if needed. This also removes duplication since Vector can share the same implementation now.
As an added bonus, you can use
Chain.traverseViaChain(0 until 100000)(f)since Range is an IndexedSeq as well, which is nice.It would be nice to get this in before 2.2.0 so we aren't stuck with the mutable API.