-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
SI-6318 fixes ClassTag.unapply for primitives #1265
Conversation
review by @jsuereth |
LGTM |
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/367/ |
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1075/ |
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/367/ |
Aborted build? |
That wasn't me. PLS REBUILD ALL |
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/370/ |
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/370/ |
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1075/ |
PLS REBUILD ALL |
@jsuereth could you please take another look? I stumbled upon an issue with patmat and had to completely redo the implementation |
Does the issue pop up with any value class or just primitives? Could you add an anyval test? Something simple like class X(val y: Int) extends AnyVal I'm paranoid about them now. Other than that LGTM, although it seems odd. Can you describe what virtpatmat was freaking out on? Im |
Only with primitives, yet I have a test that checks derived value classes: The patmat was saying: Here's the dump of -Xprint:specialize (the phase before explicitouter) for when I added the type parameter to the signature of unapply:
|
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/376/ |
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1083/ |
I c. So we couldn't just use Any because patmat freaks out with casting Patch LGTM |
Lgtm to me too. Sorry for the email shared accounts. |
My pull request has been approved by the kitteh! |
jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1083/ |
jenkins job pr-rangepos: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/376/ |
ClassTag.unapply now has overloads for primitive value classes so that it can preserve boxiness when performing subtyping tests. First I wanted to annotate ClassTag.unapply with a ClassTag itself, i.e. to transform its signature from "def unapply(x: Any): Option[T]" to "def unapply[U: ClassTag](x: U): Option[T]". But then virtpatmat_typetag.scala exhibited a nasty problem. When pattern matching with this unapply, patmat first infers U as something and then tries to pattern match against this inferred type. And if U gets inferred as an abstract type itself, bad things happen: warning: The outer reference in this type test cannot be checked at run time. That's why I decided to drop the ClassTag idea and go with 9 extra overloads. Not very beautiful, but definitely robust.
Omg that's embarassing. PLS REBUILD ALL |
Started jenkins job pr-rangepos at https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/377/ |
Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1084/ |
jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/1084/ |
jenkins job pr-rangepos: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-rangepos/377/ |
SI-6318 fixes ClassTag.unapply for primitives
ClassTag.unapply now takes a class tag itself, so that it can preserve
boxiness of value classes when performing subtyping tests.