1108949 - RegExp(RegExp object, flags) no longer throws

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[ziyunfei]

js> RegExp(/foo/,"gi")
/foo/gi
js> RegExp(/foo/uy,"gui")
/foo/giu
js> RegExp(/foo/uy,"")
/foo/uy

Comment 1

[ziyunfei]

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

This should be /foo/

Comment 2

[ziyunfei]

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

Comment 3

[ziyunfei]

Attachment: bug-1108949-v1.patch

Comment 4

[Till Schneidereit [: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"?

Comment 5

[ziyunfei]

Attachment: bug-1108949-v2.patch

Comment 6

[ziyunfei]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9da49b4541a6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce25752becd

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ce25752becd

Comment 9

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp
https://developer.mozilla.org/en-US/Firefox/Releases/39#JavaScript