Arithmetic and bitwise instructions. - #2495
Conversation
gbrail
left a comment
gbrail
left a comment
There was a problem hiding this comment.
Happy to move this through more quickly. There do seem to be some inconsistencies though -- let me know how you want to address these until we get test coverage up for this feature.
|
|
||
| @Override | ||
| public void interpret(Context cx, CallFrameV2 frame) { | ||
| Object rObj = rhs.retrieve(cx, frame); |
There was a problem hiding this comment.
This doesn't check isDouble so if that optimization is in place then DOUBLE_MARK will appear instead of the numeric value.
There was a problem hiding this comment.
Good catch. It required a little jumping through hoops to get a test case that wasn't reduced to a constant earlier., But function f(x) { let y = typeof x == 'string' ? 'bob' : 0.5; return hello ${y}!; } triggered this.
|
|
||
| @Override | ||
| public Instruction simplify(InstructionSimplification simplifier) { | ||
| KnownType rhsType = rhs.getKnownType(simplifier); |
There was a problem hiding this comment.
AI claims that a ClassCastException can be caused by this bit of code with an expression like:
("a" - 2) + 3
There was a problem hiding this comment.
Just tested this (including added debug output to check stuff is getting triggered) and it works fine. I'll generate a bunch of unit tests round this known type stuff and simplification that we can push later once the compiler and everything is in place.
| rValue = ScriptRuntime.toInt32(rObj); | ||
| } | ||
|
|
||
| long value = ScriptRuntime.toUint32(lValue) >>> ((int) rValue & 0x1F); |
There was a problem hiding this comment.
AI comment makes sense here so I'll paste it:
UnsignedRightShift wrong shift count for large doubles:
UnsignedRightShift.java:46
Uses a raw (int) rValue & 0x1F instead of ScriptRuntime.toInt32(rValue). Java's narrowing cast saturates instead of wrapping mod 2³², so: 1 >>> 2**40 → 0 (v1 and spec: 1); 1 >>> Infinity → 0 (v1 and spec: 1). v1 uses stack_int32 (Interpreter.java:1877) which goes through toInt32. The non-double path (line 43) is correct; only the double path is.
There was a problem hiding this comment.
Oh, yeah. I see the potential problem there. It seems to work, but it also feels like it should be clear it works.
49ed2e4 to
5476aed
Compare
|
This looks good, I'll merge it soon, thanks! |
2 participants
I'm grouping these two commits together partly because they feel fairly closely related in that they all operator instructions, and because we've already gone round various review aspects with the first two commits so I'm hoping we can accelerate landing these a little.
I got my robot friend to apply all the review comments from the first two PRs to these instructions.