Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

for .. in array.length not getting updated when I reset index #1910

Closed
Meai1 opened this Issue Dec 4, 2011 · 11 comments

Comments

Projects
None yet
6 participants

Meai1 commented Dec 4, 2011

for item, i in arr
   arr.splice(0, 1)
   i = 0
  # we get undefined item errors
Collaborator

michaelficarra commented Dec 4, 2011

We want the index to be mutable within a loop iteration and immutable between loop iterations. So the compilation should look something more like this:

for v, k  in arr
  # body
var k, item, _i, _len;
for (k = _i = 0, _len = arr.length; _i < _len; k = _i++) {
  v = arr[k];
  // body
}
Collaborator

michaelficarra commented Dec 4, 2011

related: #1804

Meai1 commented Dec 9, 2011

I just want to be able to remove items in a loop from the array that is being looped over. How else would I do this in Coffescript? Your example is just the exact same loop that I already provided, without the stuff that I care about.

erisdev commented Dec 9, 2011

@Meai1 create a new array, skipping the elements you don't want.

oldArray = [0..9]
newArray = for x, i in oldArray
  continue unless shouldKeep(x, i)
  x

Alternatively, iterate backward over the array and remove the elements. No index manipulation needed.

array = [0..9]
for i in [(array.length - 1)..0]
  array.splice(i, 1) unless shouldKeep(array[i], i)
Collaborator

michaelficarra commented Dec 9, 2011

@erisdiscord: newArray = (x for x, i in oldArray when shouldKeep x, i)

erisdev commented Dec 9, 2011

@michaelficarra Oops! You're absolutely right. I usually use underscore.js so I don't use array comprehensions as much as I probably should.

Owner

jashkenas commented Dec 14, 2011

So -- this is actually by design. CoffeeScript array comprehensions were supposed to use a cached iteration. (@michaelficarra, I think you're heading off in a different direction in this ticket.)

But I can see how it would be preferable to remove our optimization when we can tell that the index is being monkeyed with. Should we add logic to change the current loop control:

for (i = 0, _len = arr.length; i < _len; i++) {

Into this instead:

for (i = 0; i < arr.length; i++) {

... if we detect that i has been assigned to within the loop body? I'm not entirely sure.

Collaborator

TrevorBurnham commented Dec 14, 2011

I'd much prefer to keep the current behavior. Using for to iterate through a collection while you're modifying that collection is rather sketchy, isn't it? The loop at the top of this issue should really be written as

while arr.length > 0
  item = arr[0]
  # do stuff with item...
  arr.splice 0, 1
Owner

jashkenas commented Dec 14, 2011

Ok then -- let's keep the current behavior ... and re-open the ticket if people have some really good use cases for this that can't be written efficiently in other ways.

@jashkenas jashkenas closed this Dec 14, 2011

michaelficarra added a commit to michaelficarra/coffee-script that referenced this issue Dec 21, 2011

michaelficarra pushed a commit to michaelficarra/coffee-script that referenced this issue Dec 21, 2011

Merge pull request #1959 from jashkenas/issue1910
fixes #1910: loop index should be mutable within a loop iteration and immutable between loop iterations
Collaborator

satyr commented Dec 21, 2011

How does that patch fix the OP case? It seems irrelevant to this issue.

$ git log -1 --oneline
8728706 Merge pull request #1959 from jashkenas/issue1910

$ bin/coffee -s
  arr = [1, 2, 3]
  for item, i in arr
    arr.shift()
    i = 0
    console.log item.toString()

1
3
TypeError: Cannot call method 'toString' of undefined
Collaborator

michaelficarra commented Dec 21, 2011

It doesn't. It fixes my initial response to the OP. We decided that the OP case isn't something we need to worry about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment