Closed Bug 1054755 Opened 6 years ago Closed 5 years ago

Implement ES6 Symbol.match and IsRegExp intrinsic operation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: till, Assigned: arai)

References

()

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(4 files)

Should be straight-forward to implement, as I don't think we need to change all the String.prototype methods' implementations to work in terms of this. I'm not entirely sure what it's supposed to be used for, but there seems to be consensus about it being useful.
Blocks: 1119779
No longer blocks: es6
This was renamed.

Arai, perhaps you'd be interested in working on this, given that you're working on several patches in the same general area right now?
Flags: needinfo?(arai_a)
Summary: Implement ES6 Symbol.isRegExp → Implement ES6 Symbol.match and IsRegExp intrinsic operation
Yeah, it's interesting :)
I'll work on this after current RegExp bugs are fixed.
Flags: needinfo?(arai_a)
Prepared 4 patches.

Part 0 renames existing IsRegExp function to IsRegExpObject, to avoid name confliction and confusion.

Part 1 adds Symbol.match.

Part 2 implements IsRegExp native function (currently IsRegExp is not used in Self-hosted script).

Part 3 changes IsObjectWithClass call in String.prototype.{startsWith,endsWith} to IsRegExp, and adds IsRegExp to String.prototype.contains.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83eab3bf55d9


Then, IsRegExp is also used in RegExp constructor, with some more related changes, I'll work on it in separated bug.
  http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp-pattern-flags
Assignee: nobody → arai.unmht
Attachment #8581899 - Flags: review?(till)
Attachment #8581900 - Flags: review?(till)
Attachment #8581901 - Flags: review?(till)
Comment on attachment 8581899 [details] [diff] [review]
Part 0: Rename existing IsRegExp to IsRegExpObject.

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

Thank you for splitting this out into its own patch.
Attachment #8581899 - Flags: review?(till) → review+
Comment on attachment 8581900 [details] [diff] [review]
Part 1: Add Symbol.match.

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

As always, excellent test code!

::: js/src/jsapi.h
@@ +4287,5 @@
>  GetSymbolDescription(HandleSymbol symbol);
>  
>  /* Well-known symbols. */
>  enum class SymbolCode : uint32_t {
> +    iterator,                       // well-knowns

It's a bit weird that this comment is now for multiple lines. Perhaps just add `// Symbol.foo` comments to each line and a comment above all the well-knowns `// Well-known symbols.`?
Attachment #8581900 - Flags: review?(till) → review+
Comment on attachment 8581902 [details] [diff] [review]
Part 3: Use IsRegExp in String.prototype.{contains,startsWith,endsWith}.

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

And again, excellent tests, and very nice thorough updating of the spec steps; thank you!
Attachment #8581902 - Flags: review?(till) → review+
Blocks: 1147817
Thank you for reviewing :)
can I get a review for Part 2 as well?
Comment on attachment 8581901 [details] [diff] [review]
Part 2: Implement IsRegExp.

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

Huh, I had reviewed this, but apparently something went wrong when I sent it off. My apologies for the delay that caused!
Attachment #8581901 - Flags: review?(till) → review+
Whiteboard: [DocArea=JS]
Thanks, :arai!

I also added https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/match
It could use a review. I didn't come up with a real use case for this as I am not sure I get what it's useful for.

Also, 
> RegExp.prototype[Symbol.match] === function
> false

Is this filed somewhere?
(In reply to Florian Scholz [:fscholz] (MDN) from comment #15)
> Thanks, :arai!
> 
> I also added
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Symbol/match
> It could use a review. I didn't come up with a real use case for this as I
> am not sure I get what it's useful for.
> 
> Also, 
> > RegExp.prototype[Symbol.match] === function
> > false
> 
> Is this filed somewhere?

Thanks, I found bug 887016 for it and all other new RegExp.prototype methods.
I have almost-working patches for @@match, @@search, and @@replace, but they depends on bug 1108382, so it won't come shortly.

For now, it's almost useless, but once all of those methods are implemented, custom RegExp-like object could be made, I guess.
You need to log in before you can comment on or make changes to this bug.