RegExp(RegExp object, flags) no longer throws

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: 446240525, Assigned: 446240525)

Tracking

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

Trunk
mozilla39
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

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

Updated

5 years ago
Keywords: dev-doc-needed
Assignee

Comment 1

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

This should be /foo/
Assignee

Comment 2

5 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
Closed: 4 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.