1215430 - The RegExp creation code is a stateful, unintelligible mess

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The patch to make RegExp subclassable tries to thread new.target stuff through code, but the statefulness of RegExpObjectBuilder is confusing.  Given the "lazy" creation, what happens if two different prototypes are passed to a single REOB at different times?  It all got pretty ugly to read, and for no real value, as the code can be written just fine to not have this problem.

Two patches here.

The first splits CompileRegExpObject into the two methods that call it: regexp_construct and regexp_compile.  Bizarrely, if I un-common this code I *eliminate* ten lines overall.  So the commoning, probably meant to reduce LOC, doesn't.  o_O  I did eliminate the no-arguments optimization; I argue this is perfectly fine because |new RegExp("", "")| is useless enough that there's no point optimizing it.

The second eliminates REOB and basically follows the spec, as far as function naming and the like goes.  This patch somehow is a 40-line reduction.  O_o

Honestly, I didn't expect either patch to save lines, but it's nice to be rewarded that way, in addition to having simpler, cleaner code.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Inline the guts of the shared method implementing |new RegExp(...)| and |RegExp.prototype.compile| into each method

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Refactor RegExp code to be more spec-like in its ordering of things, and eliminate the confusing statefulness of RegExpObjectBuilder

Comment 3

[Eric Faust [:efaust]]

Comment on attachment 8674728 [details] [diff] [review]
Inline the guts of the shared method implementing |new RegExp(...)| and |RegExp.prototype.compile| into each method

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

Please also remove enum RegExpCreationMode, which I believe is now unused.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 8674729 [details] [diff] [review]
Refactor RegExp code to be more spec-like in its ordering of things, and eliminate the confusing statefulness of RegExpObjectBuilder

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

Nice simplification.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8009ed0eb3a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc949d6e3aaa

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8009ed0eb3a6
https://hg.mozilla.org/mozilla-central/rev/bc949d6e3aaa