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

Minify some Logical Expression patterns #227

Merged
merged 2 commits into from
Nov 3, 2016
Merged

Minify some Logical Expression patterns #227

merged 2 commits into from
Nov 3, 2016

Conversation

boopathi
Copy link
Member

No description provided.

@kangax
Copy link
Member

kangax commented Oct 27, 2016

This looks awesome @boopathi

@kangax
Copy link
Member

kangax commented Oct 27, 2016

/cc @hzoo @shinew for another pair of eyes

@@ -124,6 +124,54 @@ module.exports = ({ types: t }) => {
],
},

LogicalExpression(path) {
const left = path.get("left");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: declarations can be moved closer to usage.

};
const FALSY = (input) => {
// NaN and undefined are falsy
if (input.isIdentifier()) {
Copy link
Contributor

@shinew shinew Oct 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed because the .isPure() method doesn't work for NaN and undefined yet, since !undefined and !NaN are true? Are there any other such special instances?

if (result.match) {
// here we are sure that left.evaluate is always confident becuase
// it satisfied one of TRUTHY/FALSY paths
path.replaceWith(result.value(t.valueToNode(left.evaluate().value), right.node));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth caching/re-using the result of left.evaluate()? I don't know how expensive these evaluations generally are.

@@ -124,6 +124,54 @@ module.exports = ({ types: t }) => {
],
},

LogicalExpression(path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels these simplifications should be done in a post-order traversal (i.e. on exit) -- does this resolve properly for nested conditions?

e.g. (3 || foo()) || bar() should simplify to just 3.

expect(transform(sources[i])).toBe(sources[i]);
}
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test cases for nested logical expressions?

@boopathi boopathi added the Tag: New Feature Pull Request adding a new feature label Oct 31, 2016
@kangax kangax merged commit fd640d1 into master Nov 3, 2016
@kangax kangax deleted the logical-exp-0 branch November 3, 2016 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tag: New Feature Pull Request adding a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants