2000092 - Add initial implementation for IteratorChunks

Status: NEW

(Core :: JavaScript: Standard Library, enhancement, P3)

Description

[Dan Minor [:dminor]]

We should be able to implement iterator chunking in a similar way to other iterator helpers found in Iterator.js. Let's use IteratorTake as an example.

A first step is copying the relevant lines from the specification as comments into your implementation of IteratorChunks, e.g.

  // Step 1. Let O be the this value.
  // Step 2. If O is not an Object, throw a TypeError exception.
  // Step 3. Let iterated be the Iterator Record { [[Iterator]]: O, [[NextMethod]]: undefined, [[Done]]: false }.
  // Step 4. If chunkSize is not an integral Number in the inclusive interval from 1𝔽 to 𝔽(2**32 - 1), then
  // Step 4.a. Let error be ThrowCompletion(a newly created RangeError object).
  // Step 4.b. Return ? IteratorClose(iterated, error).
  // Step 5. Set iterated to ? GetIteratorDirect(O).
  // ...

Then we implement the specification, line by line, using IteratorTake for inspiration, e.g.

  // Step 1. Let O be the this value.
  var iterator = this;
  // Step 2. If O is not an Object, throw a TypeError exception.
  if (!IsObject(iterator)) {
    ThrowTypeError(JSMSG_OBJECT_REQUIRED, iterator === null ? "null" : typeof iterator);
  }
  // Step 3. Let iterated be the Iterator Record { [[Iterator]]: O, [[NextMethod]]: undefined, [[Done]]: false }.
  // Step 4. If chunkSize is not an integral Number in the inclusive interval from 1𝔽 to 𝔽(2**32 - 1), then
  // This part will be similar to IteratorTake, but use the error message JSMSG_INVALID_CHUNKSIZE, since we're using a different limit.

  // Step 5. (Inlined call to GetIteratorDirect.)
  var nextMethod = iterator.next;

  // ...

We'll need to add a definition for the JSMSG_INVALID_CHUNKSIZE, it doesn't exist. You can copy the definition of JSMSG_NEGATIVE_LIMIT and update the message.

For Step 6, we'll define a IteratorChunksGenerator like IteratorTakeGenerator, but for now, let's just do something like this:

function* IteratorChunksGenerator(iterator, nextMethod, chunkSize) {
    IteratorClose(iterator);
    return;

We can worry about implementing the actual chunking in a follow on bug. The rest of the implementation of IteratorChunks should hopefully be very similar to IteratorTake.

We should be able to write some simple tests for this. You can use the tests for Iterator.range as an inspiration. Just test things like throwing exceptions for invalid arguments.

This is a more complicated bug, so please feel free to reach out for help if you feel stuck. One good way of doing this is to push your patches to Phabricator using moz-phab with the --work-in-progress argument, then we can take a look at your code and try to help out that way, without it being a formal review request. You can also leave a needinfo for me on this bug and I'll help here.

Comment 1

[Dan Minor [:dminor]]

Hi Meg, here's the next step for implementing iterator chunking :)

Comment 2

[Meg Ford]

Thanks, will take a look this weekend :)

Comment 3

[Meg Ford]

Hi, I finished most of a draft of this, but I'm wondering two things about the tests:

1. Should I put the test file in a new directory like take.js, etc, or should I put it in the Iterator directory?
2. The test file I added doesn't seem to run. I added // |reftest| shell-option(--enable-iterator-chunking) skip-if(!Iterator.hasOwnProperty('chunks')) at the top of the file, but I'm not sure which mach command to use to run it. I tried ./mach jstests Iterator --enable-iterator-chunking and added a false == true to the file to see if I got a failure, but I think the command is wrong and the test isn't running. Thanks!

Comment 4

[Dan Minor [:dminor]]

Hi Meg, thanks for working on this :)

1. I guess for consistency, please put it in the iterator/prototype/chunking directory, you could just call it chunking.js.
2. I think you need to check Iterator.prototype.hasOwnProperty('chunks') since it's on the prototype.

Comment 5

[Meg Ford]

Attachment: Bug 2000092 - Add initial implementation for IteratorChunks. r?dminor

Comment 6

[Dan Minor [:dminor]]

Hi Meg, I'll have a look at your work-in-progress today, sorry for the delay, I was travelling last week.

Comment 7

[Dan Minor [:dminor]]

I've left some comments on your work-in-progress, please submit it for review when you're ready :)
I'll leave a needinfo here because I'm not certain that phabricator will send you any notifications for comments on a draft revision.

Comment 8

[Meg Ford]

Thanks! I submitted the patch now. Let me know if you need any other changes to it!

Comment 9

[Meg Ford]

Attachment: Bug 2000092 - Add initial implementation for IteratorChunks. r?dminor

Comment 10

[Meg Ford]

(In reply to Meg Ford from comment #8)

Thanks! I submitted the patch now. Let me know if you need any other changes to it!

Sorry, mach-bootstrap didn't install node correctly on my system so the lining commands weren't working correctly. I had to re-run linting and re-submit the patch. Hopefully the last push will pass linting.