RegExp(RegExp object, flags) no longer throws

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: 446240525, Assigned: 446240525)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla39
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
js> RegExp(/foo/,"gi")
/foo/gi
js> RegExp(/foo/uy,"gui")
/foo/giu
js> RegExp(/foo/uy,"")
/foo/uy
(Assignee)

Updated

4 years ago
Keywords: dev-doc-needed
(Assignee)

Comment 1

4 years ago
(In reply to ziyunfei from comment #0)
> js> RegExp(/foo/uy,"")
> /foo/uy

This should be /foo/
(Assignee)

Comment 2

4 years ago
Should `regexp.compile(regexp, flags)` also no longer throws just like the RexExp constructor did in rev29 for consistency?

https://bugs.ecmascript.org/show_bug.cgi?id=3505
(Assignee)

Updated

4 years ago
Assignee: nobody → 446240525
Summary: RegExp(regexp, flags) no longer throws → RegExp(RegExp object, flags) no longer throws
(Assignee)

Comment 3

4 years ago
Posted patch bug-1108949-v1.patch (obsolete) — Splinter Review
Attachment #8568387 - Flags: review?(till)
Comment on attachment 8568387 [details] [diff] [review]
bug-1108949-v1.patch

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

Very nice. r=me with feedback addressed.

::: js/src/builtin/RegExp.cpp
@@ +144,4 @@
>   */
>  static bool
>  CompileRegExpObject(JSContext *cx, RegExpObjectBuilder &builder, CallArgs args,
> +                    RegExpStaticsUse staticsUse, bool callerIsCompile)

Among the nice usage of enums here, this bool sticks out quite badly. Please introduce a `RegExpCreationMode` enum with the entries "CreateForCompile" and "CreateForConstruct". Or, because I don't particularly like these names but can't think of anything else, something better.

@@ +161,5 @@
>  
>      RootedValue sourceValue(cx, args[0]);
>  
>      /*
> +     * When args[0] is a RegExp object.

Just get rid of this comment altogether. It doesn't add any value, anymore.

@@ +166,5 @@
>       */
>      if (IsObjectWithClass(sourceValue, ESClass_RegExp, cx)) {
>          /*
> +         * If the caller is RegExp.prototype.compile and args[1] is not
> +         * undefined, then throw a TypeError.

"For RegExp.prototype.compile, if the first argument is a RegExp object, the second argument must be undefined. Otherwise, throw a TypeError."

@@ +192,5 @@
> +        sourceAtom = g->getSource();
> +
> +        /*
> +         * If args[1] is not undefined, then parse the 'flags' from args[1].
> +         * Otherwise, extract the 'flags' out of sourceObj.

nit: s/out of/from/

@@ +198,2 @@
>          RegExpFlag flags;
> +        if (args.hasDefined(1)) {

Keep the anonymous block around all this. The idea is for the RegExpGuard to be as short-lived as possible.

@@ +203,2 @@
>                  return false;
> +            args[1].setString(flagStr);

I tried understanding why this exists, and found the explanation in http://hg.mozilla.org/mozilla-central/rev/4003aacbc675. In short, before the RootedFoo things came along, rooting worked in indirect, weird ways. Setting the arg to the just-created string value forced it to be rooted on the stack.

In these modern, enlightened days, we don't need stuff like this anymore, so you can remove this line and the equivalent below (in line 232 with your patch, I guess).

::: js/src/tests/ecma_6/RegExp/bug1108949.js
@@ +1,1 @@
> +assertEq(RegExp(/foo/my).flags, "my");

Please rename this file to something that's understandable without visiting bugzilla. Perhaps "flag-params-handling.js"?
Attachment #8568387 - Flags: review?(till) → review+
(Assignee)

Comment 5

4 years ago
Attachment #8568387 - Attachment is obsolete: true
Attachment #8575774 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ce25752becd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.