SI-9516 Fix the behavior of Int shift Long operations. #5117
Conversation
|
Review by @lrytz |
| @@ -102,13 +102,13 @@ abstract class ConstantFolder { | |||
| case nme.XOR => Constant(x.longValue ^ y.longValue) | |||
| case nme.AND => Constant(x.longValue & y.longValue) | |||
| case nme.LSL if x.tag <= IntTag | |||
| => Constant(x.intValue << y.longValue) | |||
| => Constant(x.intValue << y.longValue.toInt) // .toInt required to bootstrap despite SI-9516 | |||
There was a problem hiding this comment.
maybe change this comment, something like TODO: remove .toInt once starr is includes the fix for SI-9516 (2.12.0-M5)
sjrd
force-pushed
the
SI9516-int-shift-long
branch
from
4ae66da to
9057bf6
Compare
In any shift operation where the lhs is an Int (or smaller) and the rhs is a Long, the result kind must be Int, and not Long. This is important because the lhs must *not* be promoted to a Long, as that causes an opcode for long shift to be emitted. This uses an rhs modulo 64, instead of int shifts which use an rhs module 32. Instead, the rhs must be downgraded to an Int. The new behavior is consistent with the same operations in the Java programming language.
|
LGTM! |
|
I don't think I was paying attention in the related fix. My comment here is that the test could be junit. |
|
@som-snytt As I explained on scala/scala-dev#136, If I make it a JUnit test, it won't succeed if run from sbt. In other words, it would break |
|
I don't want to seem rude but I wonder how this breaking change wasn't mentioned in 2.12 release notes? Does complete list of behavior changes (i.e. same code compiles without a warning but gives different result) exist somewhere? Without it it seems impossible to upgrade 2.11 -> 2.12 on existing project w/o 100% tests coverage (including 3rd-party libraries). Thanks in advance for the answer. |
|
I added the |
In any shift operation where the lhs is an Int (or smaller) and
the rhs is a Long, the result kind must be Int, and not Long.
This is important because the lhs must not be promoted to a
Long, as that causes an opcode for long shift to be emitted.
This uses an rhs modulo 64, instead of int shifts which use an
rhs module 32. Instead, the rhs must be downgraded to an Int.
The new behavior is consistent with the same operations in the
Java programming language.
Link to SI-9516: https://issues.scala-lang.org/browse/SI-9516