Closed Bug 1120170 Opened 5 years ago Closed 5 years ago

Self-host RegExp.prototype.flags


(Core :: JavaScript: Standard Library, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: 446240525, Assigned: 446240525)


(Depends on 1 open bug)



(2 files, 3 obsolete files)

No description provided.
I think this is a good decision, yes.
Blocks: 1120151
Attached patch bug-1120170-v1.patch (obsolete) — Splinter Review
Assignee: nobody → 446240525
Attachment #8567499 - Flags: review?(till)
Comment on attachment 8567499 [details] [diff] [review]

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 */
> +
> +// ES6 draft rev34 (2015/02/20) 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.

Attached patch bug-1120170-v2.patch (obsolete) — Splinter Review
Attachment #8567499 - Attachment is obsolete: true
Attachment #8567584 - Flags: review?(till)
Attached patch bug-1120170-v3.patch (obsolete) — Splinter Review
Attachment #8567584 - Attachment is obsolete: true
Attachment #8567584 - Flags: review?(till)
Attachment #8567586 - Flags: review?(till)
Comment on attachment 8567586 [details] [diff] [review]

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+
Change JSMSG_NOT_NONNULL_OBJECT message to accept an argument
Attachment #8567586 - Attachment is obsolete: true
Attachment #8567644 - Flags: review+
Attachment #8567645 - Flags: review+
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.