-
Notifications
You must be signed in to change notification settings - Fork 291
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
Add DestructArray and DestructArrayAndReassign operations #300
Add DestructArray and DestructArrayAndReassign operations #300
Conversation
75eedcc
to
7ef54dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! The main thing I'm still wondering about is whether we really need the AndReassign
variant of this. On the one hand, it most likely gets converted early on in the JS pipeline into a destruct and regular reassignments, so probably doesn't really cover any new code. On the other hand, compiling a destruct + reassign to a normal destruct and a bunch of reassignments is kind of inefficient, and so if that happens frequently enough, it's probably worth keeping the additional instruction for that. WDYT?
Let me compare the bytecode generate from |
4cac874
to
1fb20d1
Compare
I think we should support the const v4 = [15,30,"Hello"];
let v5 = 1000;
[v5] = v4;
//VS
const v4 = [15,30,"Hello"];
let v5 = 1000;
v5 = v4[0]; The bc from the two samples are linked below: The second reason is that handling |
Wrt to nested arrays in destruct operations, I think we can handle this during compilation using temporary variables. For example, if we have the following program: let [x,[a,b]] = [1,[2,3]] we can re-write this during compilation to: let [x, _temp0] = [1,[2,3]]
let [a,b] = _temp0 Diffing the bytecode dumps from the orginal and modified sample above doesn't reveal a significant difference so I think we should be fine on that front as well. All other review comments have been addressed so we should be good to go here. Let me know if you'd like me to incorporate any other changes or improvements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool thanks, that makes sense, so let's keep both variants!
return Array(outputs) | ||
} | ||
|
||
public func destructAndReassign(_ candidates: [Variable], atIndices indices: [Int], from input: Variable, hasRestElement: Bool = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe change signature to something like destructAndReassign(_ input: Variable, selecting indices: [Int], into: outputs [Variable], hasRestElement: Bool)
to better match destruct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on renaming this function to just destruct
and updating the signature as you've suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, that works as well!
This implementation currently does not support the following JS constructs:
Nested arrays (i.e. [[a,b],c] = arr)To be handled during compilationholes (i.e [a,,b] = arr)rest element (i.e. [a, ...b] = arr)