Change Painless Tree Structure for Variable/Method Chains#19459
Change Painless Tree Structure for Variable/Method Chains#19459jdconrad wants to merge 20 commits intoelastic:masterfrom
Conversation
followed by a regular execution of the script.
| * used during the writing phase to dup stack values from this storeable as | ||
| * necessary during certain store operations. | ||
| */ | ||
| abstract int size(); |
There was a problem hiding this comment.
Should we call it storeSize()? Maybe elaborate on this? I find it confusing looking at different nodes, to know they are really correct. And we don't want dup bugs...
There was a problem hiding this comment.
Renamed this to accessElementCount and gave a more thorough explanation of what the number actually means in the comments.
simplify the expression nodes in Painless.
|
@rmuir I removed the branching shortcuts. Not sure it actually simplified that much, but it does remove the necessity to write a jump in a child node to a label that gets written in a parent node. Note this does write out multiple branches in the byte code now, but since you checked the actual assembly that gets written is the same, good to go. |
| void analyze(Locals locals) { | ||
| final Definition.Type returnType; | ||
| final Type returnType; | ||
| final List<String> actualParamTypeStrs; |
There was a problem hiding this comment.
do you think this is the right tradeoff in this file? It causes the much longer org.objectweb.asm.Type many times in the file, including making lines so long they must be unnaturally split. Maybe we should rename Definition.Type to something that does not conflict? I would also rename Compiler to something that does not conflict with java.lang.Compiler! We can do this separately, but these conflicts are a hassle...
There was a problem hiding this comment.
Isolated, for this specific file, it would definitely be a poor trade-off. For the project as it stands right now, I think this is the correct one since the majority (all?) of node files have the static import as Definition.Type. I believe it's confusing when switching between nodes (at least for me it was). Longer term, though, I'm 100% on board with renaming the classes in Painless to get conflict-free names. It definitely sucks having both a Painless Type and and ASM Type. Originally there was a bit more separation when writing was in a different set of files from analysis. For now, I'm going to leave this, and let's rename Definition.Type in the next refactor PR which should probably address more pieces of Definition as well. I believe Uwe had some suggestions there.
|
this cleanup is good. I left two minor comments. |
|
@rmuir Thanks for the review! Contrary to what github believes, this has been merged into master. |
The following changes have been made to Painless in this PR:
Changed the tree structure for variable/method chains. Previously all loads were required to have a parent of EChain. ALink nodes and EChain were removed in favor of allowing constants and variables to be expression nodes instead. Postfix nodes have replaced non-primary pieces (call, field, and brace) with the parent-child relationship of the final postfix at the top with a child prefix until a primary becomes a leaf node. Example: x.call()[0] becomes Postfix([0]) --> Postfix(call) --> Primary(x).
The grammar has been updated to allow proper postfixes to be attached to all primaries including constants and precedence. Example: (x ? map1 : map2).get('string')
Instead of possible node rewriting for primaries and postfixes, sub nodes have been added and the primary or postfix will have a sub node as a child in some cases to help break up the analysis and writing of certain logical breaks such. Example: PCallInvoke --> PSubDefCall/PSubCallInvoke instead of PDefCall/PCallInvoke replacing PCallInvoke in the tree altogether.
Clean up of variable permissions in nodes attempting to make variables private when possible.
Added a way to prevent picky (ambiguity) tests for testing of actual error message output the user will see when testing for lexer/parser errors.