691695 - Refactor RegExp components as prep for overhaul

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: WIP: reorg before lazifying.

+++ This bug was initially created as a clone of Bug #673274 +++

The code for the RegExp implementation wasn't well segregated. I talked to Luke today about the way that this should be organized (similar to vm/StringObject) and I belted out the attached patch, which is passing shell tests, but still needs some cleanup.

This patch introduces:

vm/RegExpObect
    - Extends JSObject with reserved slots accessors, js::Class (RegExpClass)
      vtable methods
    - RegExpPrivate, which is the C++ blobby that can be shared and holds code.
      This lives in the private slot of the RegExpObject

vm/RegExpStatics
    - VM data structure for the static regular expression results. These live
      in 1:1 correspondence with GlobalObject, but they have their own
      JSObject for finalization of the C++ blobby.

builtin/RegExp
    - Stuff necessary for performing js_InitRegExpClass; i.e. all the property
      and method implementation that gets stuck on in |RegExp.prototype|.
      This is a client of the vm data structures.

Comment 1

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: RegExp component reorg.

Aside from moving code around and renaming things, a few other small refactoring-like bits slipped in. (Sorry that it's a single patch, but these things just kind of started happening naturally as I defined the new interfaces.)

- Strengthened the RegExpObject invariant that it must have a RegExpPrivate if
  it has been properly initialized. (Uses PreInitRegExpObject guard thing to
  enforce this at several of the initialization sites.)
- Changed all the uses of RegExp flags. Compare the betterness of
  JSREG_FOLD vs IgnoreCaseFlag. (Statically asserted to be the same values.)
- Made GlobalObject return a pointer from getRegExpStatics instead of Value.
- I got rid of the horrible "Swap" terminology in the builtins and
  made everything simply "reset" -- sometimes you reset with statics, and
  sometimes you reset without statics. Reset is like "reinitialize", and exists
  because RegExps can't be normal children due to bug 691682 and its kin.

Passes shell tests and browser seem to work okay, so r? and pushing to try.

Comment 2

[Chris Leary [:cdleary] (not checking bugmail)]

Also just realized that replacePrivate is vestigial.

Comment 3

[Chris Leary [:cdleary] (not checking bugmail)]

Bah, and the second header declaration for ResetRegExpObject is missing a RegExpObject parameter (doesn't affect anything though, since it's declared/defined properly in the inlines file).

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

Cloning added a lot of people to this relatively unimportant refactoring bug. Fixing that, feel free to add yourself back if I've removed you in error.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 564771 [details] [diff] [review]
RegExp component reorg.

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

Looks great.

::: js/src/builtin/RegExp.cpp
@@ +487,5 @@
> +
> +    return true;
> +}
> +
> +namespace js {

I don't think it's yet documented, but the style we've converged towards is, for namespace'd function definitions, to write js::foo instead of namespace js { foo }.  This has several benefits, so it would be good to do it here.

::: js/src/jsparse.cpp
@@ +8881,5 @@
>          } else {
> +            reobj = RegExpObject::createNoStatics(
> +                      context, tokenStream.getTokenbuf().begin(),
> +                      tokenStream.getTokenbuf().length(),
> +                      RegExpFlag(tokenStream.currentToken().t_reflags), &tokenStream);

Instead of of the two long blobs, could you compute the parameters into locals thereby making RegExpObject::create(NoStatics) each fit on a single line?  That avoids the problem that the indentation is not SM-style.

::: js/src/jsregexpinlines.h
@@ +100,5 @@
> +    return !v.isPrimitive() && v.toObject().isRegExp();
> +}
> +
> +inline bool
> +IsRegExpMetaChar(jschar c)

Can you re-evaluate, for reach of these inline functions, whether it really has to go in the -inl.h file?  Ideally, more of these would be statics in the .cpp or non-inline externs in the .h.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to Luke Wagner [:luke] from comment #5)
> Can you re-evaluate, for reach of these inline functions, whether it really
> has to go in the -inl.h file?  Ideally, more of these would be statics in
> the .cpp or non-inline externs in the .h.

I promise I'll do this in a followup patch! I've filed bug 692750 -- the issue I have is that I've made a queue on top of this patch (due to the surrounding bugs) and moving function bodies to the source file will prevent diffs from carrying. Maybe there's a better way of handling this in my future workflow?

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/4d312cb93a94