tweak do-while loops #1452

alexlamsl · 2017-01-30T21:22:12Z

Much more straightforward than #1451 - just adding do{...}while(false) and consolidating AST_While optimisations.

alexlamsl · 2017-01-30T21:27:29Z

Previously optimising AST_While calls the whole code block from AST_DWLoop. Might as well just move these few lines into AST_DWLoop instead.

AST_While is being directly substituted by a AST_For. if_break_in_loop() will be called within AST_For so no need to duplicate the efforts.

kzc · 2017-01-31T17:03:33Z

lib/compress.js

@@ -1749,8 +1749,13 @@ merge(Compressor.prototype, {
                    extract_declarations_from_unreachable_code(compressor, self.body, a);
                    return make_node(AST_BlockStatement, self, { body: a });
                }
+            } else {


Please add comment: // self instanceof AST_Do

kzc · 2017-01-31T17:11:33Z

lib/compress.js

-        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);


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?

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.

I see. Thanks.

kzc · 2017-02-01T01:32:55Z

lib/compress.js

@@ -1749,7 +1749,7 @@ merge(Compressor.prototype, {
                    extract_declarations_from_unreachable_code(compressor, self.body, a);
                    return make_node(AST_BlockStatement, self, { body: a });
                }
-            } else {
+            } else { // self instanceof AST_Do


Ugh - comment after a brace! I'll get over it.

I could do } else /*if (self instanceof AST_Do)*/ { if that pleases the gods... 😉

Gosh, no. On its own line please:

} else { // self instanceof AST_Do ...

kzc · 2017-02-01T01:34:16Z

LGTM. Ready to squash commits.

- `do{...}while(false)` => `{...}` - clean up `AST_While` logic

- `do{...}while(false)` => `{...}` - clean up `AST_While` logic closes mishoo#1452

kzc · 2017-03-02T14:43:51Z

@alexlamsl In light of #1532 can we revert this PR?

I don't have confidence in break analysis in both while(){} and do{}while() loops.

See: #1452 (comment)

alexlamsl · 2017-03-02T15:05:57Z

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.

alexlamsl · 2017-03-02T15:07:52Z

lib/compress.js

@@ -1749,8 +1749,14 @@ merge(Compressor.prototype, {
                    extract_declarations_from_unreachable_code(compressor, self.body, a);
                    return make_node(AST_BlockStatement, self, { body: a });
                }
+            } else {
+                // self instanceof AST_Do
+                return self.body;


@kzc this is the line which I believe is causing #1532 instead of the section below.

return self; would certainly fix the do-while case in #1532. I've already confirmed this.

But I am not certain about the handling of breaks and continues in while(){} loops with this PR. There's insufficient test case coverage.

I'll get rid of this and add new test cases to address #1532. Then we can revisit this optimisation at a later date.

alexlamsl force-pushed the loops branch from 1077373 to d364a25 Compare January 31, 2017 03:42

alexlamsl mentioned this pull request Jan 31, 2017

improve string concatenation #1454

Closed

kzc reviewed Jan 31, 2017

View reviewed changes

kzc reviewed Feb 1, 2017

View reviewed changes

tweak do-while loops

6a09157

- `do{...}while(false)` => `{...}` - clean up `AST_While` logic

alexlamsl force-pushed the loops branch from fb72bb4 to 6a09157 Compare February 1, 2017 01:54

alexlamsl mentioned this pull request Feb 1, 2017

Next release after v2.7.5 #1383

Closed

alexlamsl added a commit to alexlamsl/UglifyJS that referenced this pull request Feb 18, 2017

tweak do-while loops

c6f6abe

- `do{...}while(false)` => `{...}` - clean up `AST_While` logic closes mishoo#1452

alexlamsl closed this in ae4db00 Feb 23, 2017

alexlamsl deleted the loops branch February 24, 2017 00:19

alexlamsl commented Mar 2, 2017

View reviewed changes

kzc mentioned this pull request Mar 2, 2017

disable do{...}while(false) optimisation #1534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tweak do-while loops #1452

tweak do-while loops #1452

alexlamsl commented Jan 30, 2017

alexlamsl commented Jan 30, 2017

kzc Jan 31, 2017

alexlamsl Feb 1, 2017

kzc Jan 31, 2017

alexlamsl Feb 1, 2017

kzc Feb 1, 2017

kzc Feb 1, 2017

alexlamsl Feb 1, 2017

kzc Feb 1, 2017

alexlamsl Feb 1, 2017

kzc commented Feb 1, 2017

kzc commented Mar 2, 2017

alexlamsl commented Mar 2, 2017 •

edited

Loading

alexlamsl Mar 2, 2017

kzc Mar 2, 2017 •

edited

Loading

kzc Mar 2, 2017

alexlamsl Mar 2, 2017

tweak do-while loops #1452

tweak do-while loops #1452

Conversation

alexlamsl commented Jan 30, 2017

alexlamsl commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzc commented Feb 1, 2017

kzc commented Mar 2, 2017

alexlamsl commented Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

kzc Mar 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlamsl commented Mar 2, 2017 •

edited

Loading

kzc Mar 2, 2017 •

edited

Loading