Move variable slotting to the IR tree#51452
Conversation
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
rjernst
left a comment
There was a problem hiding this comment.
Looks good, a couple minor suggestions
|
|
||
| public class DoWhileLoopNode extends LoopNode { | ||
|
|
||
| public DoWhileLoopNode() { |
There was a problem hiding this comment.
why do we need an explicit empty ctor the same as the default ctor?
There was a problem hiding this comment.
Good catch. Missed this during the merge.
| methodWriter.visitVarInsn(MethodWriter.getType(variable.clazz).getOpcode(Opcodes.ISTORE), variable.getSlot()); | ||
| methodWriter.visitVarInsn(variable.getAsmType().getOpcode(Opcodes.ISTORE), variable.getSlot()); | ||
|
|
||
| Variable loop = scopeTable.getVariable("#loop"); |
There was a problem hiding this comment.
Maybe we could have another method for special/private/hidden (whatever we want to call them) variables? I'd rather have a dedicated method than have to remember # is outside our grammar and so this cannot collide with user variables.
There was a problem hiding this comment.
Added defineInternalVariable and getInternalVariable to address this specific issue. Let me know if you'd prefer a different name, but I like "internal" because I think it makes it clear this is something defined by the compiler rather than a user.
|
@rjernst Thanks for the review! |
|
@stu-elastic I'd like to continue to make progress on this, so I'm going to go ahead and merge with the approval from @rjernst. When you have more time, I'm happy to go over the code here and make follow up PR changes if necessary. |
Currently, variable slotting (a variable's index on the ASM stack) is generated by the user tree during semantic checking. With the split it no longer makes sense to have the user tree generate a variable's slot. This PR moves variable slot allocation to the IR tree using the class ScopeTable. This class holds variables currently in scope and calculates an appropriate slot for them. This also allows for optimization phases to occur on the IR tree because now variables can be injected without having to update slotting.
For review the most important class to look at in this PR is the newly added ScopeTable. The majority of this PR is boilerplate adding ScopeTable to the IR nodes write method signature.