[
https://issues.apache.org/jira/browse/GROOVY-12098?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18090805#comment-18090805
]
ASF GitHub Bot commented on GROOVY-12098:
-----------------------------------------
Copilot commented on code in PR #2621:
URL: https://github.com/apache/groovy/pull/2621#discussion_r3456945890
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java:
##########
@@ -1119,36 +1133,54 @@ public void evaluatePrefixMethod(final PrefixExpression
expression) {
private void evaluatePrefixMethod(final int op, final String method, final
Expression expression, final Expression orig) {
// load expressions
- VariableSlotLoader usesSubscript = loadWithSubscript(expression);
+ SubscriptTemps subscript = loadWithSubscript(expression);
// execute method
- execMethodAndStoreForSubscriptOperator(op, method, expression,
usesSubscript, orig);
+ execMethodAndStoreForSubscriptOperator(op, method, expression,
subscript, orig);
// new value is already on stack, so nothing to do here
- if (usesSubscript != null)
controller.getCompileStack().removeVar(usesSubscript.getIndex());
+ if (subscript != null) {
+ CompileStack compileStack = controller.getCompileStack();
+ compileStack.removeVar(subscript.index.getIndex());
+ compileStack.removeVar(subscript.receiver.getIndex());
+ }
}
- private VariableSlotLoader loadWithSubscript(final Expression expression) {
+ private SubscriptTemps loadWithSubscript(final Expression expression) {
AsmClassGenerator acg = controller.getAcg();
// if we have a BinaryExpression, check if it is with subscription
if (expression instanceof BinaryExpression bexp) {
if (bexp.getOperation().getType() == LEFT_SQUARE_BRACKET) {
+ OperandStack operandStack = controller.getOperandStack();
+ CompileStack compileStack = controller.getCompileStack();
+
+ // GROOVY-12098: evaluate the receiver once (and before the
index, to keep
+ // left-to-right order) and store it, so the read (getAt
below) and the
+ // later write (putAt) reuse the value instead of
re-evaluating the receiver.
+ Expression object = bexp.getLeftExpression();
+ object.visit(acg);
+ ClassNode objectType = operandStack.getTopOperand();
+ if (objectType.isGenericsPlaceHolder() ||
GenericsUtils.hasPlaceHolders(objectType)) {
+ objectType =
controller.getTypeChooser().resolveType(object, controller.getClassNode());
+ }
+ int objectId = compileStack.defineTemporaryVariable("$object",
objectType, true);
+ VariableSlotLoader objectExpression = new
VariableSlotLoader(objectType, objectId, operandStack);
Review Comment:
`"$object"` is a vague temporary name for what is semantically the subscript
*receiver*. Renaming it to something like `"$receiver"` would better match the
surrounding terminology (`SubscriptTemps.receiver`) and reduce confusion when
debugging/reading generated locals.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java:
##########
@@ -1119,36 +1133,54 @@ public void evaluatePrefixMethod(final PrefixExpression
expression) {
private void evaluatePrefixMethod(final int op, final String method, final
Expression expression, final Expression orig) {
// load expressions
- VariableSlotLoader usesSubscript = loadWithSubscript(expression);
+ SubscriptTemps subscript = loadWithSubscript(expression);
// execute method
- execMethodAndStoreForSubscriptOperator(op, method, expression,
usesSubscript, orig);
+ execMethodAndStoreForSubscriptOperator(op, method, expression,
subscript, orig);
// new value is already on stack, so nothing to do here
- if (usesSubscript != null)
controller.getCompileStack().removeVar(usesSubscript.getIndex());
+ if (subscript != null) {
+ CompileStack compileStack = controller.getCompileStack();
+ compileStack.removeVar(subscript.index.getIndex());
+ compileStack.removeVar(subscript.receiver.getIndex());
+ }
}
Review Comment:
The fix explicitly changes both postfix and prefix paths to reuse cached
receiver/index temps; the new `@CompileStatic` receiver-single-evaluation test
currently covers postfix (`receiver()[1]++`) but not prefix/decrement with a
non-idempotent receiver. Adding a `@CompileStatic` test asserting
`++receiver()[1]` (and/or `--receiver()[1]`) only calls the receiver once would
help prevent regressions specifically in the prefix pipeline.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java:
##########
@@ -1059,28 +1059,42 @@ private void evaluateNotInstanceof(final
BinaryExpression expression) {
);
}
+ // Holds the temporaries created for a subscript expression's receiver and
index so
+ // that a read-modify-write such as a[i]++ reuses them for the getAt
(read) and the
+ // putAt (write) instead of evaluating the receiver (or index) expression
twice.
+ private static final class SubscriptTemps {
+ final VariableSlotLoader receiver, index;
+ SubscriptTemps(final VariableSlotLoader receiver, final
VariableSlotLoader index) {
+ this.receiver = receiver;
+ this.index = index;
+ }
+ }
+
private void evaluatePostfixMethod(final int op, final String method,
final Expression expression, final Expression orig) {
CompileStack compileStack = controller.getCompileStack();
OperandStack operandStack = controller.getOperandStack();
// load Expressions
- VariableSlotLoader usesSubscript = loadWithSubscript(expression);
+ SubscriptTemps subscript = loadWithSubscript(expression);
// save copy for later
operandStack.dup();
ClassNode expressionType = operandStack.getTopOperand();
int tempIdx = compileStack.defineTemporaryVariable("postfix_" +
method, expressionType, true);
// execute method
- execMethodAndStoreForSubscriptOperator(op, method, expression,
usesSubscript, orig);
+ execMethodAndStoreForSubscriptOperator(op, method, expression,
subscript, orig);
// remove the result of the method call
operandStack.pop();
// reload saved value
operandStack.load(expressionType, tempIdx);
compileStack.removeVar(tempIdx);
- if (usesSubscript != null)
compileStack.removeVar(usesSubscript.getIndex());
+ if (subscript != null) {
+ compileStack.removeVar(subscript.index.getIndex());
+ compileStack.removeVar(subscript.receiver.getIndex());
+ }
Review Comment:
Cleanup of `SubscriptTemps` locals is now duplicated in both postfix and
prefix methods. Consider extracting a small helper (e.g.,
`removeSubscriptTemps(SubscriptTemps)`) to keep the removal order consistent in
one place and reduce the chance of future mismatches as this logic evolves.
> Pre/post increment on array element double-evaluates the receiver
> -----------------------------------------------------------------
>
> Key: GROOVY-12098
> URL: https://issues.apache.org/jira/browse/GROOVY-12098
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
