1120170 - Self-host RegExp.prototype.flags

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Comment 1

[Till Schneidereit [:till]]

I think this is a good decision, yes.

Comment 2

[ziyunfei]

Attachment: bug-1120170-v1.patch

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8567499 [details] [diff] [review]
bug-1120170-v1.patch

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

Nice! I'll get your fix for bug 1133387 landed in Shumway today, too, so we won't have to worry about that.

::: js/src/builtin/RegExp.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// ES6 draft rev34 (2015/02/20) 21.2.5.3 get RegExp.prototype.flags
> +function RegExpGetterFlags() {

nit: I think I'd prefer "RegExpFlagsGetter".

@@ +25,5 @@
> +        result += "m";
> +
> +    // Steps 13-15.
> +    if (R.unicode)
> +        result += "u";

We should ignore the `unicode` flag for now, see bug 1132295. Please comment it out here with a reference to bug 1135377.

Bonus points for commenting out line 15 of tests/ecma_6/RegExp/flags.js and introducing a canary test similar to line 10 in that file.

::: js/src/js.msg
@@ +51,5 @@
>  MSG_DEF(JSMSG_READ_ONLY,               1, JSEXN_TYPEERR, "{0} is read-only")
>  MSG_DEF(JSMSG_CANT_DELETE,             1, JSEXN_TYPEERR, "property {0} is non-configurable and can't be deleted")
>  MSG_DEF(JSMSG_NOT_FUNCTION,            1, JSEXN_TYPEERR, "{0} is not a function")
>  MSG_DEF(JSMSG_NOT_CONSTRUCTOR,         1, JSEXN_TYPEERR, "{0} is not a constructor")
> +MSG_DEF(JSMSG_NOT_OBJECT,              1, JSEXN_TYPEERR, "{0} is not an object")

While we already have JSMSG_NOT_NONNULL_OBJECT, I like this much better because it prints the value in question. I think it should still say "non-null object", though. r=me on changing JSMSG_NOT_NONNULL_OBJECT to include the value and changing the other (11, I just checked) call-sites accordingly. Feel free to ignore this, though.

Comment 4

[Till Schneidereit [:till]]

(In reply to Till Schneidereit [:till] from comment #3)
> Nice! I'll get your fix for bug 1133387 landed in Shumway today, too, so we
> won't have to worry about that.

Err, actually Arai's fix[1], sorry for the confusion.

[1] https://github.com/mozilla/shumway/pull/2081

Comment 5

[ziyunfei]

Attachment: bug-1120170-v2.patch

Comment 6

[ziyunfei]

Attachment: bug-1120170-v3.patch

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8567586 [details] [diff] [review]
bug-1120170-v3.patch

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

Thank you, this is most excellent.

I have one request that I feel pretty bad about because I should've made it earlier: can you split out the error message changes into their own patch that we can land as part 1 here? That way the two things are clearly separated. It looks to me like it should be pretty straight-forward to do this split, so need to ask for a re-review.

Comment 8

[ziyunfei]

Attachment: bug-1120170-part-1.patch

Change JSMSG_NOT_NONNULL_OBJECT message to accept an argument

Comment 9

[ziyunfei]

Attachment: bug-1120170-part-2.patch

Comment 10

[ziyunfei]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9339c39c9513

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b66941d0e328
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2dcbff898a1

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b66941d0e328
https://hg.mozilla.org/mozilla-central/rev/f2dcbff898a1

Comment 13

[Jason Orendorff [:jorendorff]]

In part 1, I think we're leaking the result of DecompileValueGenerator in most of the places where it's called. The comment on the declaration says:

>  * The caller must call JS_free on the result after a successful call.

Also, JSDVG_SEARCH_STACK should not be used in new code. In the case of WeakMap_construct, for example, the value we wish to complain about wasn't produced by any expression in the source code. If by some awful chance DVG does find that value on the operand stack, the error message will contain a misleading expression.

We need better tools for building good error messages generally; DecompileValueGenerator is hard to use safely, and ValueToSource *can't* be used safely. :-\

Comment 14

[Jason Orendorff [:jorendorff]]

Oh --- till, I set ni?you in the hopes that you might take a moment to fix this, or help arai determine what to do.

Comment 15

[Jason Orendorff [:jorendorff]]

It would also be good to use a function instead of copying code and pasting it many places.

Comment 16

[Till Schneidereit [:till]]

Sigh, yes, stupid me. Filed bug 1136153, will fix.