Closed Bug 1215430 Opened 5 years ago Closed 5 years ago

The RegExp creation code is a stateful, unintelligible mess


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox44 --- fixed


(Reporter: Waldo, Assigned: Waldo)




(2 files)

The patch to make RegExp subclassable tries to thread 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 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.
Attachment #8674728 - Flags: review?(efaustbmo) → review+
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.
Attachment #8674729 - Flags: review?(efaustbmo) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.