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

polish: skip creating extra reference for safely re-used node #10720

Merged
merged 2 commits into from Nov 16, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 15, 2019

Q                       A
Fixed Issues? Fixes #10717
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

In this PR we skip creating extra reference when the left of ?? is a static reference, which is safely to reused when later unfolding into conditional comparison.

Compared to the master branch, this PR generate less code for the common case var bar = foo ?? "bar"
Before:

var bar = (_foo = foo) !== null && _foo !== void 0 ? _foo : "bar"
// minified (52 B)
// var bar=null!==(_foo=foo)&&void 0!==_foo?_foo:"bar";

After:

var bar = foo !== null && foo !== void 0 ? foo : "bar"
// minified (43 B)
// var bar=null!==foo&&void 0!==foo?foo:"bar";

@JLHwung JLHwung added PR: Polish 💅 A type of pull request used for our changelog categories Spec: Nullish Coalescing Operator labels Nov 15, 2019
scope.push({ id: ref });
let ref, assignment;
// skip creating extra reference when `left` is semantically safe to re-use
if (t.isIdentifier(node.left)) {
Copy link
Member

Choose a reason for hiding this comment

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

What about using isConstantExpression, or maybe better maybeGenerateMemoised?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I end up with maybeGenerateMemoised because it is rare that the left is a literal --even if it is, creating a reference other than inlining this literal should costs less bytes in most situations, given that we generate [_]+ as identifier name for literals.

ref = node.left;
assignment = t.cloneNode(node.left);
} else {
assignment = t.assignmentExpression("=", ref, node.left);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if ref is non-null, it is always cloned from node.left.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

We need to figure out why CircleCI isn't running for your PRs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories Spec: Nullish Coalescing Operator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@babel/plugin-proposal-null-coalescing-operator injects unnecessary variable
3 participants