Looks good. One thing I'm noting is that String is currently handled as
ObjectType (i.e. getType().isObject() will return true), which is true
in Java terms, but in this context we should treat String as primitive,
right?
Hannes
Am 2013-10-14 21:59, schrieb André Bargull:
"primitive type" was a bad choice of words, I rather meant
"side-effect-free".
When all operands are primitives, most operations won't trigger
side-effects. Only most operations because in `0 - (0 instanceof 1)`
all operands are primitives, but `(0 instanceof 1)` still needs to be
executed before the subtraction. So I was thinking of the following
change for Codegen (incomplete -> `instanceof` etc. not handled!):
private static boolean isSideEffectFree(final Expression rhs) {
if (rhs instanceof LiteralNode.PrimitiveLiteralNode) {
return true;
} else if (rhs instanceof IdentNode) {
return !rhs.getSymbol().isScope() &&
!rhs.getType().isObject();
} else if (rhs instanceof UnaryNode) {
UnaryNode unary = (UnaryNode) rhs;
return isSideEffectFree(unary.rhs());
} else if (rhs instanceof BinaryNode) {
BinaryNode binary = (BinaryNode) rhs;
return isSideEffectFree(binary.lhs())
&& isSideEffectFree(binary.rhs());
} else if (rhs instanceof TernaryNode) {
TernaryNode ternary = (TernaryNode) rhs;
return isSideEffectFree(ternary.getTest())
&& isSideEffectFree(ternary.getTrueExpression())
&& isSideEffectFree(ternary.getFalseExpression());
}
return false;
}
private static boolean safeLiteral(final Expression rhs) {
// return isSideEffectFree(rhs);
return rhs instanceof LiteralNode && !(rhs instanceof
ArrayLiteralNode);
}
- André
Side effects can still occur with primitive types. As an example, this
script from the test case as a numeric right hand side:
({valueOf: function(){throw 0}}) - ({valueOf: function(){throw 1}} - 1)
But maybe something like this (pseudo-code) could work in
CodeGenerator#safeLiteral:
isPrimitiveLiteralNode || (isIdentNode && isPrimitiveType)
An IdentNode can have side effects if it is a global with a getter, but
if we know it's type that shouldn't be the case.
Hannes
Am 2013-10-09 18:44, schrieb André Bargull:
>/ Quick question for this change set:
/>/
/>/ Why does CodeGenerator#safeLiteral(...) need to test for literal nodes
/>/ instead of using type information? When the right-hand-side is a
/>/ primitive type, side effects cannot occur, so the additional
/>/ swap/dup/pop instructions can be omitted safely.
/>/
/>/ For example `function f(a){var c = 1; return a - c }` now emits:
/>/ ALOAD 1
/>/ ILOAD 4
/>/ SWAP
/>/ INVOKESTATIC jdk/nashorn/internal/runtime/JSType.toNumber
/>/ (Ljava/lang/Object;)D
/>/ DUP2_X1
/>/ POP2
/>/ I2D
/>/ DSUB
/>/
/>/ Using type info it's possible to change the bytecode to:
/>/ ALOAD 1
/>/ INVOKESTATIC jdk/nashorn/internal/runtime/JSType.toNumber
/>/ (Ljava/lang/Object;)D
/>/ ILOAD 4
/>/ I2D
/>/ DSUB
/>/
/>/
/>/ (Also: That was one of the bug reports which worried me a bit, because
/>/ I expected the changes to be non-trivial. :-(
/>/
/>/
/>/ - André/
