-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Move AStatement mutable members into isolated Input/Output objects #53348
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
@elasticmachine run elasticsearch-ci/2 |
* infinite loops during runtime. | ||
*/ | ||
int statementCount = 0; | ||
// TODO: remove placeholders once analysis and write are combined into build |
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.
Can you ref issues in TODO's like this? eg TODO: ..., see #12345
.
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.
void analyze(ScriptRoot scriptRoot, Scope scope) { | ||
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) { | ||
this.input = input; | ||
output = new Output(); |
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.
Consider creating output close to it's first use, at line 68.
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.
Done.
void analyze(ScriptRoot scriptRoot, Scope scope) { | ||
Output analyze(ScriptRoot scriptRoot, Scope scope, Input input) { | ||
this.input = input; | ||
output = new Output(); |
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.
Same as above, consider changing where this creation occurs. It's about ~50 lines away from actual use.
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.
Done.
@stu-elastic Thanks for the review! Will address your comments soon. |
@rjernst Thanks for the review. Will commit as soon as CI completes. |
This is the follow up to #53075, isolating the mutable data on the statement nodes as the referenced change did the expression nodes.
This is a step toward the long-term goal of making the "user" trees nodes immutable. This change isolates the mutable data for statement nodes in the "user" tree during the semantic (analysis) phase by moving the mutable data into Input and Output objects. These objects are created locally during the semantic phase to share information required for semantic checking between parent-child nodes.
Note that for this change, Input and Output are still stored in a mutable way on the statement nodes. This will not be the case once the semantic (analysis) phase and ir generation (write) phase are combined in a future change.
Relates #49869