-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify constant handling in Painless #52612
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
@@ -72,6 +72,7 @@ StatementExpressionNode write(ClassNode classNode) { | |||
|
|||
statementExpressionNode.setLocation(location); | |||
statementExpressionNode.setMethodEscape(methodEscape); | |||
statementExpressionNode.setNoop(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After speaking with you (@stu-elastic) and @rjernst I've decided to change this a bit. I've removed noop
in favor of doPop
which will pop extraneous values off the stack when necessary such as a method call with side effects, but an ignored return value somecall();
.
I also removed the need for StatementExpressionNode to handle return values. Instead, SExpression will just directly translate to a ReturnNode if a return is required. This moves the responsibility to where is makes sense instead of having redundant returns in multiple nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I continued to iterate on this after more discussion with @rjernst . I removed the doPop and will instead use an expression's type to determine if a pop is required or not (popping the void type is the same as a noop in this case).
@Override | ||
public void write(ClassWriter classWriter, MethodWriter methodWriter, Globals globals, ScopeTable scopeTable) { | ||
public void write(ClassWriter classWriter, MethodWriter methodWriter, ScopeTable scopeTable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's now MemberFieldLoadNode
and MemberFieldStoreNode
where Store
has the following new code:
if (isStatic == false) {
methodWriter.loadThis();
}
getChildNode().write(classWriter, methodWriter, scopeTable);
Why the new code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new code is to handle putting the value on the stack that will be written to the field. So store requires an expression to generate a value to store where as load does not.
callSubNode.setExpressionType(Pattern.class); | ||
callSubNode.setBox(Pattern.class); | ||
callSubNode.setMethod(new PainlessMethod( | ||
Pattern.class.getMethod("compile", String.class, int.class), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this being written to clinit
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh perhaps in classNode.addFieldNode(fieldNode);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classNode.addFieldNode
creates a declaration for a member field.
These lines actually add the regex constant generation and assignment to the clinit method:
BlockNode blockNode = classNode.getClinitNode().getBlockNode();
blockNode.addStatementNode(statementExpressionNode);
|
||
import static org.elasticsearch.painless.WriterConstants.CLASS_TYPE; | ||
|
||
public class MemberFieldStoreNode extends UnaryNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minimal javadoc please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
if (clinitNode.getBlockNode().getStatementsNodes().isEmpty() == false) { | ||
ReturnNode returnNode = new ReturnNode(); | ||
returnNode.setLocation(new Location("internal$clinit$return", 0)); | ||
clinitNode.getBlockNode().addStatementNode(returnNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this when creating the clinit block, and have an inner block that we expose as the one that statements can be added to? This way we don't manipulate the IR tree when calling write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to do no modifications of the ir tree from write. I just do the method writer for clinit and the return statement in the ClassNode. I'll consider better ways to do this, but for now this makes the most sense because it's the simplest change.
@rjernst @stu-elastic Thanks for the initial rounds of review. |
@rjernst I think this is ready for another look. I believe I addressed all your comments and continued iterating on the best way to handle StatementExpressionNode - see Stu's initial comments on this for my follow ups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@rjernst Thanks for the second review. |
This change makes constant handling simpler by doing the following: