1054755 - Implement ES6 Symbol.match and IsRegExp intrinsic operation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

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.

Comment 1

[Till Schneidereit [:till]]

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?

Comment 2

[Tooru Fujisawa [:arai]]

Yeah, it's interesting :)
I'll work on this after current RegExp bugs are fixed.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Rename existing IsRegExp to IsRegExpObject.

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

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add Symbol.match.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Implement IsRegExp.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use IsRegExp in String.prototype.{contains,startsWith,endsWith}.

Comment 7

[Till Schneidereit [: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.

Comment 8

[Till Schneidereit [:till]]

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.`?

Comment 9

[Till Schneidereit [:till]]

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!

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
can I get a review for Part 2 as well?

Comment 11

[Till Schneidereit [:till]]

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!

Comment 12

[Tooru Fujisawa [:arai]]

Thank you again!
I'll post patches to bug 1147817 shortly.

https://hg.mozilla.org/integration/mozilla-inbound/rev/708f251fadc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9336398400c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb1d70016c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2fc40b10421

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/708f251fadc5
https://hg.mozilla.org/mozilla-central/rev/9336398400c5
https://hg.mozilla.org/mozilla-central/rev/2fb1d70016c0
https://hg.mozilla.org/mozilla-central/rev/e2fc40b10421

Comment 14

[Tooru Fujisawa [:arai]]

Updated following documents:
  https://developer.mozilla.org/en-US/Firefox/Releases/40
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla

Comment 15

[Florian Scholz (Open Web Docs)]

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?

Comment 16

[Tooru Fujisawa [:arai]]

(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.