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.
--
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]
