[bug#11687] Don't rebuild scala.Seq to drop elems in unapplySeq #8340
Conversation
| @@ -1,3 +1,3 @@ | |||
| object Test extends App{ | |||
| LazyList.from(1) match { case LazyList(1, 2, x @_*) => println("It worked!") } | |||
| LazyList.from(1) match { case LazyList(1, 2, x @_*) => println(s"It worked! (class: ${x.getClass.getSimpleName})") } | |||
There was a problem hiding this comment.
this change to prevent DCE
There was a problem hiding this comment.
Does that mean that the match expression was not even evaluated because of DCE?
There was a problem hiding this comment.
I think, based on @smarter's comment, just the x @_* (which is UnapplySeqWrapper#drop) wasn't evaluated?
There was a problem hiding this comment.
patmat never generates the code in the first place if the symbol is unused.
diesalbla
added
the
library:collections
| def drop(n: Int): scala.Seq[A] = c.view.drop(n).toSeq | ||
| def drop(n: Int): scala.Seq[A] = c match { | ||
| case seq: scala.Seq[A] => seq.drop(n) | ||
| case _ => c.view.drop(n).toSeq |
There was a problem hiding this comment.
In which situation is it actually a good idea to go through a view here ?
There was a problem hiding this comment.
going through a view avoids building an unnecessary intermediate collection (with the same type as c)
There was a problem hiding this comment.
Right, but that's only true for types which aren't Seq but use UnapplySeqWrapper. Which types are those exactly? Do we have test cases that exercise this code path ?
There was a problem hiding this comment.
we don't. and they will still be forced.
I think saying that it ought to be LazyList.from(c.view.drop(n)) (or LazyList.from(c).drop(n)) to preserve laziness is valid
There was a problem hiding this comment.
I can hear @dwijnand's voice in my head asking if this is really all necessary and the best design
There was a problem hiding this comment.
Right, but that's only true for types which aren't Seq but use UnapplySeqWrapper. Which types are those exactly?
All the mutable.Seq leaves.
|
Thanks @NthPortal 👍 |
Fixes scala/bug#11687