Skip to content
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

warn & drop #__PURE__ iff IIFE is dropped #1511

Merged
merged 1 commit into from
Feb 27, 2017

Conversation

alexlamsl
Copy link
Collaborator

@kzc so I think this is the proper fix for #1509 (comment), i.e. without dropping the warning altogether.

New test under test/mocha/minify.js won't fail on master, but will fail on harmony without the fix in this PR. When this PR is merged on top of #1509, all test/compress tests pass.

@kzc
Copy link
Contributor

kzc commented Feb 27, 2017

@alexlamsl The fix looks good. Thanks.

Any idea why this problem did not occur on master without this PR?

Do you plan to apply this PR to master as well?

@alexlamsl
Copy link
Collaborator Author

The PR is targetted at master - I plan to merge master onto #1509 after this.

I think it's down to the fact that on master when has_side_effects() is called in the tests, it will eventually eliminate the side-effect-free AST_Node, so it all works out okay. But that isn't necessarily the case, and looks like something on harmony did exactly that.

There are many places where has_side_effects() is called but the expression is retained even on master, though those sites are normally not where you'd put an IIFE. I will do another check across all call sites just to make myself feel more comfortable, and then I'll merge this.

@alexlamsl
Copy link
Collaborator Author

The new tests should cover all WARN: Dropping __PURE__ call cases by now.

- consolidate `side-effects` optimisations
- improve string `+` optimisation
- enhance literal & `conditionals` optimisations
@alexlamsl alexlamsl merged commit 858e6c7 into mishoo:master Feb 27, 2017
@alexlamsl alexlamsl deleted the pure-drop branch February 27, 2017 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants