Self-host RegExp.prototype.flags

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ziyunfei, Assigned: ziyunfei)

Tracking

(Depends on: 1 bug)

Trunk
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
I think this is a good decision, yes.
Blocks: 1120151
(Assignee)

Comment 2

3 years ago
Created attachment 8567499 [details] [diff] [review]
bug-1120170-v1.patch
Assignee: nobody → 446240525
Attachment #8567499 - Flags: review?(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.
Attachment #8567499 - Flags: review?(till) → review+
(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
(Assignee)

Comment 5

3 years ago
Created attachment 8567584 [details] [diff] [review]
bug-1120170-v2.patch
Attachment #8567499 - Attachment is obsolete: true
Attachment #8567584 - Flags: review?(till)
(Assignee)

Comment 6

3 years ago
Created attachment 8567586 [details] [diff] [review]
bug-1120170-v3.patch
Attachment #8567584 - Attachment is obsolete: true
Attachment #8567584 - Flags: review?(till)
Attachment #8567586 - Flags: review?(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.
Attachment #8567586 - Flags: review?(till) → review+
(Assignee)

Comment 8

3 years ago
Created attachment 8567644 [details] [diff] [review]
bug-1120170-part-1.patch

Change JSMSG_NOT_NONNULL_OBJECT message to accept an argument
Attachment #8567586 - Attachment is obsolete: true
Attachment #8567644 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8567645 [details] [diff] [review]
bug-1120170-part-2.patch
Attachment #8567645 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b66941d0e328
https://hg.mozilla.org/mozilla-central/rev/f2dcbff898a1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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. :-\
Flags: needinfo?(till)
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.
It would also be good to use a function instead of copying code and pasting it many places.
Depends on: 1136153
Sigh, yes, stupid me. Filed bug 1136153, will fix.
Flags: needinfo?(till)
You need to log in before you can comment on or make changes to this bug.