Closed Bug 1108949 Opened 6 years ago Closed 5 years ago

RegExp(RegExp object, flags) no longer throws

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: 446240525, Assigned: 446240525)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(1 file, 1 obsolete file)

js> RegExp(/foo/,"gi")
/foo/gi
js> RegExp(/foo/uy,"gui")
/foo/giu
js> RegExp(/foo/uy,"")
/foo/uy
Keywords: dev-doc-needed
(In reply to ziyunfei from comment #0)
> js> RegExp(/foo/uy,"")
> /foo/uy

This should be /foo/
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: nobody → 446240525
Summary: RegExp(regexp, flags) no longer throws → RegExp(RegExp object, flags) no longer throws
Attached 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+
Attachment #8568387 - Attachment is obsolete: true
Attachment #8575774 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ce25752becd
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.