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

enhance evaluate #3714

Merged
merged 1 commit into from
Feb 12, 2020
Merged

enhance evaluate #3714

merged 1 commit into from
Feb 12, 2020

Conversation

alexlamsl
Copy link
Collaborator

@kzc hopefully more productive than intentionally breaking test/compress.js 😉

@kzc
Copy link
Contributor

kzc commented Feb 12, 2020

Sure - intentionally is the operative word. Break stuff the old fashioned way.

I don't follow this PR. No new compress option was introduced, so presumably it's intended to be a safe optimization. What does it mean to ignore side effects in a loop/if/switch/case condition? In which situations are they safe to ignore?

@alexlamsl
Copy link
Collaborator Author

alexlamsl commented Feb 12, 2020

Traditionally AST_Node.evaluate() bails out on expressions like x(), 42 or a = 42. But there are various transformations like, e.g

(function(a) {
    switch (1) {
      case a = 1:
        console.log(a);
        break;
      default:
        console.log(2);
        break;
    }
})(1);

which we can optimise the AST without dropping the evaluating expression.

What we want here is to inspect the constant value produced without worrying about any side effects caused by said expression.

@alexlamsl
Copy link
Collaborator Author

So optimisations of this form already exist but it only applies to a, b, 42 − with this PR we can extend to something like a, (b = (c, 42))

@alexlamsl alexlamsl merged commit dd22eda into mishoo:master Feb 12, 2020
@alexlamsl alexlamsl deleted the evaluate branch February 12, 2020 01:01
@kzc
Copy link
Contributor

kzc commented Feb 12, 2020

inspect the constant value produced without worrying about any side effects caused by said expression

Interesting. That would be a good comment for ignore_side_effects.

So only AST_Assign and AST_Sequence presently support this feature?

@kzc
Copy link
Contributor

kzc commented Feb 12, 2020

I think I get it now - you only use this in conjunction with is_truthy:

                var ev = value.is_truthy() || value.evaluate(compressor, true);

@alexlamsl
Copy link
Collaborator Author

So only AST_Assign and AST_Sequence presently support this feature?

Yes (for now) − only used to support AST_Sequence at the top level, not supports both types within nested expression as well.

@alexlamsl alexlamsl mentioned this pull request Feb 13, 2020
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.

None yet

2 participants