Conversation
|
Previously optimising
|
| extract_declarations_from_unreachable_code(compressor, self.body, a); | ||
| return make_node(AST_BlockStatement, self, { body: a }); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Please add comment: // self instanceof AST_Do
| if (!compressor.option("loops")) return self; | ||
| self = AST_DWLoop.prototype.optimize.call(self, compressor); | ||
| if (self instanceof AST_While) { | ||
| if_break_in_loop(self, compressor); |
There was a problem hiding this comment.
What is the equivalent logic for if_break_in_loop(self, compressor); in the new OPT(AST_DWLoop) code above? I see a newly added else clause for the self instanceof AST_Do case, but the AST_While logic is the same as it was. Was if_break_in_loop never needed?
There was a problem hiding this comment.
AST_For does if_break_in_loop() already, so once AST_While is turned into AST_for and .transform(compressor), it should be covered.
It is technically not calling if_break_in_loop() in the same order (or even the same number of times) as before, but I had a scan through the method and thinks this is safe to do if not a minor performance improvement.
lib/compress.js
Outdated
| return make_node(AST_BlockStatement, self, { body: a }); | ||
| } | ||
| } else { | ||
| } else { // self instanceof AST_Do |
There was a problem hiding this comment.
Ugh - comment after a brace! I'll get over it.
There was a problem hiding this comment.
I could do } else /*if (self instanceof AST_Do)*/ { if that pleases the gods... 😉
There was a problem hiding this comment.
Gosh, no. On its own line please:
} else {
// self instanceof AST_Do
...
|
LGTM. Ready to squash commits. |
- `do{...}while(false)` => `{...}`
- clean up `AST_While` logic
- `do{...}while(false)` => `{...}`
- clean up `AST_While` logic
closes mishoo#1452
|
@alexlamsl In light of #1532 can we revert this PR? I don't have confidence in See: #1452 (comment) |
|
Just sat down at my desk - let me have a look and see if it's fixable. If not, I think only the top half of the PR needs to be reverted anyway. Either case I'll come up with a new PR with those test cases from #1532. |
| } | ||
| } else { | ||
| // self instanceof AST_Do | ||
| return self.body; |
There was a problem hiding this comment.
return self; would certainly fix the do-while case in #1532. I've already confirmed this.
There was a problem hiding this comment.
But I am not certain about the handling of breaks and continues in while(){} loops with this PR. There's insufficient test case coverage.
There was a problem hiding this comment.
I'll get rid of this and add new test cases to address #1532. Then we can revisit this optimisation at a later date.
Much more straightforward than #1451 - just adding
do{...}while(false)and consolidatingAST_Whileoptimisations.