Last Comment Bug 725168 - Element iterators should use [[Get]] and not peculiarly ignore the prototype chain
: Element iterators should use [[Get]] and not peculiarly ignore the prototype ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: es6
  Show dependency treegraph
 
Reported: 2012-02-07 17:01 PST by Jason Orendorff [:jorendorff]
Modified: 2012-03-02 09:36 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (4.42 KB, patch)
2012-02-07 17:13 PST, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2012-02-07 17:01:42 PST
This only matters in dumb corner cases, but basically:

    Array.prototype[2] = "potato";
    for (let x of [0, 1, , 3])
        print(x);

should print 0, 1, "potato", 3
rather than 0, 1, undefined, 3.
Comment 1 Jason Orendorff [:jorendorff] 2012-02-07 17:13:31 PST
Created attachment 595257 [details] [diff] [review]
v1

I don't know what I was thinking. Just randomly hating on holes.

The fast path for dense arrays could be thrown away too. It really depends on how we want to implement this.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-13 17:37:59 PST
Comment on attachment 595257 [details] [diff] [review]
v1

Review of attachment 595257 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed about throwing out the dense array fast path, I think.  We can pick up fast paths when they're demonstrably necessary; in the meantime, clarity and concision are more important.

::: js/src/jsiter.cpp
@@ -1153,5 @@
> -        } else {
> -            Value v = DoubleValue(i);
> -            if (!js_ValueToStringId(cx, v, &id))
> -                goto error;
> -        }

Did I review this code and not require IndexToString?  :-(
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-02-13 17:39:10 PST
Oh, I express no position on whether it is better to have for-of look up the prototype chain (as happens with this patch) or look only at own properties (as happens currently).  Definitely whatever the rationale is should be recorded in this bug, tho.
Comment 4 Jason Orendorff [:jorendorff] 2012-02-14 13:11:43 PST
The rationale, in broad strokes:

  - it is more like the canonical for loop that way
  - nothing in the language currently does this kind of "get if own property";
    for better or worse, everything uses [[Get]]
  - just how unusual this is shows up as code complexity
  - everybody says ignoring the prototype chain won't make for-of any faster

In any case TC39 will kick this back and forth and eventually come to some decision, and we'll implement that.
Comment 5 Jason Orendorff [:jorendorff] 2012-02-25 13:31:00 PST
Incidentally, removing the "simple" fast path fixes bug 726212, so the patch I check in for that bug will just be a few tests.
Comment 6 Jason Orendorff [:jorendorff] 2012-03-01 07:05:12 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/dac3cc61a1b0
Comment 7 Marco Bonardo [::mak] 2012-03-02 06:18:39 PST
https://hg.mozilla.org/mozilla-central/rev/dac3cc61a1b0
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 09:36:36 PST
Nice diffstat!

Note You need to log in before you can comment on or make changes to this bug.