blackdrag commented on code in PR #3226:
URL: https://github.com/apache/groovy/pull/3226#discussion_r4506990204
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java:
##########
@@ -433,21 +440,57 @@ protected void assignToArray(final Expression orig, final
Expression receiver, f
// load rhs
rhsValueLoader.visit(asmGenerator);
+ // The store consumes the value. Duplicate it under the array
reference
+ // and the int index first, so the assignment expression can yield
the
+ // original value without a local variable. The copy is taken
before the
+ // component cast, which applies to the consumed value only.
+ if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
{
+ duplicateTopBeneath(operandStack, 2);
+ }
operandStack.doGroovyCast(arrayType.getComponentType());
// store value in array
bew.arraySet(false);
- // update operand stack
+ // update operand stack; a duplicated value stays as the
expression result
operandStack.remove(3);
-
- if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
- rhsValueLoader.visit(asmGenerator); // re-load result value
} else {
super.assignToArray(orig, receiver, index, rhsValueLoader, safe);
}
}
+ /**
+ * Duplicates the top operand beneath {@code operandsBelow} tracked
operands.
+ * An array store has an array reference and an int index below the value,
+ * two words in total; the value is one word, or two for long and double.
+ */
+ private void duplicateTopBeneath(final OperandStack operandStack, final
int operandsBelow) {
+ ClassNode top = operandStack.getTopOperand();
+ operandStack.remove(1);
+ ClassNode[] below = new ClassNode[operandsBelow];
+ int wordsBelow = 0;
+ for (int i = operandsBelow - 1; i >= 0; i -= 1) {
+ below[i] = operandStack.getTopOperand();
+ wordsBelow += isTwoWord(below[i]) ? 2 : 1;
+ operandStack.remove(1);
+ }
+ int wordsTop = isTwoWord(top) ? 2 : 1;
+ int opcode;
+ if (wordsTop == 1 && wordsBelow == 1) opcode = DUP_X1;
+ else if (wordsTop == 1 && wordsBelow == 2) opcode = DUP_X2;
+ else if (wordsTop == 2 && wordsBelow == 1) opcode = DUP2_X1;
+ else if (wordsTop == 2 && wordsBelow == 2) opcode = DUP2_X2;
Review Comment:
this should be in OperandStack (see dup and swap)
##########
src/test/groovy/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy:
##########
@@ -261,21 +262,73 @@ final class StaticCompilationTest extends
AbstractBytecodeTestCase {
'L0',
'LINENUMBER 3',
'ALOAD 1',
- 'ASTORE 2',
'ICONST_0',
- 'ISTORE 3',
'ICONST_1',
- 'ISTORE 4',
- 'ALOAD 2',
- 'ILOAD 3',
- 'ILOAD 4',
'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.intArraySet ([III)V',
'L1',
'LINENUMBER 4',
'RETURN'
])
}
+ // GROOVY-10657: a reference right-hand side must not be kept in a local
+ @Test
+ void testSubscriptAssignmentReference() {
+ assert compile(method: 'm', '''@groovy.transform.CompileStatic
+ void m(Object[] a, Object v) {
+ a[0] = v
+ }
+ ''').hasStrictSequence([
+ 'L0',
+ 'LINENUMBER 3',
+ 'ALOAD 1',
+ 'ICONST_0',
+ 'ALOAD 2',
+ 'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.objectArraySet
([Ljava/lang/Object;ILjava/lang/Object;)V',
+ 'L1',
+ 'LINENUMBER 4',
+ 'RETURN'
+ ])
+ }
+
+ // GROOVY-10657: the assignment value is the right-hand side, duplicated
under the store operands
+ @Test
+ void testSubscriptAssignmentValue() {
+ assert compile(method: 'm', '''@groovy.transform.CompileStatic
+ Object m(Object[] a, Object v) {
+ return (a[0] = v)
+ }
+ ''').hasStrictSequence([
+ 'L0',
+ 'LINENUMBER 3',
+ 'ALOAD 1',
+ 'ICONST_0',
+ 'ALOAD 2',
+ 'DUP_X2',
+ 'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.objectArraySet
([Ljava/lang/Object;ILjava/lang/Object;)V',
+ 'ARETURN'
+ ])
+ }
+
+ // GROOVY-10657: a category-2 value uses DUP2_X2 so the copy keeps both
words
+ @Test
+ void testSubscriptAssignmentLongValue() {
Review Comment:
actually most of these test should be not only testing static compilation,
but also have that kind of sequence without static compilation.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java:
##########
@@ -433,21 +440,57 @@ protected void assignToArray(final Expression orig, final
Expression receiver, f
// load rhs
rhsValueLoader.visit(asmGenerator);
+ // The store consumes the value. Duplicate it under the array
reference
+ // and the int index first, so the assignment expression can yield
the
+ // original value without a local variable. The copy is taken
before the
+ // component cast, which applies to the consumed value only.
+ if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
{
+ duplicateTopBeneath(operandStack, 2);
+ }
operandStack.doGroovyCast(arrayType.getComponentType());
// store value in array
bew.arraySet(false);
- // update operand stack
+ // update operand stack; a duplicated value stays as the
expression result
operandStack.remove(3);
-
- if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
- rhsValueLoader.visit(asmGenerator); // re-load result value
} else {
super.assignToArray(orig, receiver, index, rhsValueLoader, safe);
}
}
+ /**
+ * Duplicates the top operand beneath {@code operandsBelow} tracked
operands.
+ * An array store has an array reference and an int index below the value,
+ * two words in total; the value is one word, or two for long and double.
+ */
+ private void duplicateTopBeneath(final OperandStack operandStack, final
int operandsBelow) {
Review Comment:
The only value for operandsBelow seems to be 2, why do we need this to work
for another case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
