-
Notifications
You must be signed in to change notification settings - Fork 381
Enhance AstDumper for debugging and testing #10177
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
base: main
Are you sure you want to change the base?
Conversation
in multiple other places
|
Example of generating an AST dump with JS output included, while working on a fix for #9731 - running a build of the Hello sample before and after the change, and running function Button(handler){
$clinit_FocusWidget();
ButtonBase.call(this, $createPushButtonElement($doc));
($clinit_DOM() , this.element).className = 'gwt-Button' || '';
- ($clinit_DOM() , this.element).innerHTML = 'Click me' || '';
- $addDomHandler(this, handler, ($clinit_ClickEvent() , $clinit_ClickEvent() , TYPE));
+ this.element.innerHTML = 'Click me' || '';
+ $addDomHandler(this, handler, ($clinit_ClickEvent() , TYPE));
} |
|
CompilerTest doesn't fail locally, investigating... |
|
Failures appear to be an example of #9947, rerunning. |
|
|
||
| @Override | ||
| public boolean visit(JDeclarationStatement x, Context ctx) { | ||
| if (!suppressType) { |
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.
This makes suppressType unread.
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.
You're right... makes me suspicious that I "broke" this, at least in the context of for loops. I wish this had tests (since apparently it once upon a time produced compilable code, but that was many years ago), but I don't think it is critical to add them just for a debugging tool just yet...
I'll verify what our behavior is now here. My hunch is that it doesn't matter, and that we have just an assignment JBinaryExpression instead, but we'll see.
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 don't know what was the intention behind this flag and if it's still relevant (and if it is needed for for loops, is it also needed for try-with-resources?), but since this is just for debugging it should be fine to remove the flag.
| private final Set<FilterRange> sourceFiles; | ||
|
|
||
| private boolean shouldVisitCurrentTopLevelStatement = 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.
Enhances the AstDumper utility class with the aim of bringing back some of the functionality previously removed in 64f9b00, where individual methods could be traced from source Java all the way to JS output. This is a small step in that direction that nevertheless provides more insight into what the compiler is doing, through a series of changes:
More improvement remains - multiple permutations may end up mixed in the same file, different compiler passes are not individually listed or labeled, arguments are re-parsed every time they are used, but impact to the API is minimal for now.