Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Review notes #4

Closed
anba opened this issue Oct 5, 2015 · 1 comment
Closed

Review notes #4

anba opened this issue Oct 5, 2015 · 1 comment

Comments

@anba
Copy link
Collaborator

anba commented Oct 5, 2015

https://ljharb.github.io/String.prototype.matchAll/#String.prototype.matchAll

Missing ReturnIfAbrupt after step 1. (not required if steps 5-6 are placed directly after step 1)

Steps 5-6 should be placed directly after step 1.

Step 10: Calling RegExpCreate with regexp as the pattern parameter is probably not intended. This either needs to be changed to:

  1. Let pattern be ToString(Get(regexp, "source")).
  2. ReturnIfAbrupt(pattern).
  3. Let rx be RegExpCreate(pattern, flags).

Or alternatively Symbol.species should be used like in RegExp.prototype[Symbol.split] (http://tc39.github.io/ecma262/#sec-regexp.prototype-@@split, steps 5-6 and step 13). I guess Symbol.species is actually the correct choice here, even if Symbol.matchAll doesn't get added.

Nit: Missing full stop in step 15.

Note 2 is not correct, e.g. rx is exposed to user script if RegExp.prototype.exec is overridden.

https://ljharb.github.io/String.prototype.matchAll/#CreateRegExpStringIterator

It's not valid to call IsRegExp and Get in steps 1-2 because it can trigger side effects if RegExp.prototype[Symbol.match] or RegExp.prototype.global are not the intrinsic functions.

https://ljharb.github.io/String.prototype.matchAll/#%RegExpStringIteratorPrototype%.next

Missing ReturnIfAbrupt after step 6 (modified RegExp.prototype.exec!).

https://ljharb.github.io/String.prototype.matchAll/#PropertiesOfRegExpStringIteratorInstances

"IsRegExp([[IteratingRegExp]]) is always true." is not correct, same reason as above.

@ljharb
Copy link
Member

ljharb commented Oct 5, 2015

Thanks for the thorough review! I still need to update the spec text after the latest meeting - specifically, to a) accept either a string or a regex, and b) to create Symbol.matchAll so that subclassing is handled properly. I'll update this issue when I've done so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants