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

test for for-of optimization on arrays and add it for array type anno… #4747

Merged
merged 3 commits into from
Jun 26, 2017

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented Oct 17, 2016

was going to do this earlier but I closed the pr

@hzoo hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Oct 17, 2016
@hzoo hzoo force-pushed the for-of-opt branch 2 times, most recently from a8d8b9a to dced37b Compare October 17, 2016 23:17
@codecov-io
Copy link

codecov-io commented Oct 17, 2016

Codecov Report

Merging #4747 into 7.0 will increase coverage by 0.11%.
The diff coverage is 96.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##              7.0    #4747      +/-   ##
==========================================
+ Coverage   85.21%   85.33%   +0.11%     
==========================================
  Files         284      284              
  Lines        9958     9984      +26     
  Branches     2780     2788       +8     
==========================================
+ Hits         8486     8520      +34     
+ Misses        971      965       -6     
+ Partials      501      499       -2
Impacted Files Coverage Δ
.../babel-plugin-transform-es2015-for-of/src/index.js 95.49% <96.66%> (+1.37%) ⬆️
packages/babel-traverse/src/path/context.js 85.34% <0%> (-1.73%) ⬇️
packages/babel-traverse/src/visitors.js 85.71% <0%> (-0.96%) ⬇️
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.63% <0%> (ø) ⬆️
packages/babel-traverse/src/scope/index.js 79.04% <0%> (+0.23%) ⬆️
...ages/babel-traverse/src/path/inference/inferers.js 90.12% <0%> (+1.23%) ⬆️
...ackages/babel-traverse/src/path/inference/index.js 54.09% <0%> (+6.55%) ⬆️
packages/babel-types/src/flow.js 40.74% <0%> (+9.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cce83a0...0ff5946. Read the comment docs.

Copy link
Member

@DrewML DrewML left a comment

Choose a reason for hiding this comment

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

Nice!

@danez
Copy link
Member

danez commented Oct 18, 2016

This assumes that people have the correct type annotation. But what if they do what we do? Like having typeannotations but not verifying them, because then even if the type annotation is Array it could be an object.

@hzoo
Copy link
Member Author

hzoo commented Oct 19, 2016

Hm good point - would we be ok with that assumption? @jridgewell thoughts?

@hzoo
Copy link
Member Author

hzoo commented Nov 1, 2016

Linking @Kovensky's suggestion too http://astexplorer.net/#/dh8bycNK1A/2

@jridgewell
Copy link
Member

I think the biggest improvement here is when we can prove it's an array, so @Kovensky's would work just fine for me.

@hzoo
Copy link
Member Author

hzoo commented Nov 15, 2016

@hzoo
Copy link
Member Author

hzoo commented Nov 15, 2016

We could remove the type annotation check I guess and it would just check for binding/array.from

@jridgewell
Copy link
Member

How about with arguments?

@hzoo
Copy link
Member Author

hzoo commented Apr 27, 2017

@danez I think if you are going to opt-in to types, it's on you to make sure you run flow/ts. So it's a good assumption to make

@hzoo
Copy link
Member Author

hzoo commented Apr 27, 2017

via @ljharb: Array.of, new Array(), Array, String.prototype.match, RegExp.prototype.exec, and friends

@hzoo hzoo changed the base branch from master to 7.0 May 2, 2017 18:56
@@ -0,0 +1,4 @@
for (const y of (b: Array<any>)) {}
Copy link
Member Author

@hzoo hzoo Jun 9, 2017

Choose a reason for hiding this comment

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

Didn't add the type check yet but we can

@hzoo
Copy link
Member Author

hzoo commented Jun 23, 2017

I think we can land this as is since I didn't add the type part so it's safe

const binding = path.scope.getBinding(right.node.name);
return optimize(path, binding.path.get("init"));
} else if (right.isCallExpression() && (
isArrayFrom(right.get("callee").node) ||
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather see this in

export function CallExpression() {
return resolveCall(this.get("callee"));
}
.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be happy to let you finish this up if you want to refactor

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this, I'll add this to #5835, and you'll promise to review it. 😉

@hzoo hzoo merged commit bd9e186 into 7.0 Jun 26, 2017
@ljharb ljharb deleted the for-of-opt branch June 26, 2017 21:02
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
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: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants