569115 - TM: don't use tinyids in jsregexp.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 448259 [details] [diff] [review]
patch

>+#define GETTER(prop) \
>+    prop##Getter
>+
>+#define DEFINE_GETTER(prop, code)                                     \
>+    static JSBool                                                     \
>+    GETTER(prop)(JSContext *cx, JSObject *obj, jsval id, jsval *vp) { \
[...]
>+DEFINE_GETTER(SOURCE, *vp = STRING_TO_JSVAL(re->source))
>+DEFINE_GETTER(GLOBAL, *vp = BOOLEAN_TO_JSVAL((re->flags & JSREG_GLOB) != 0))
>+DEFINE_GETTER(IGNORE_CASE, *vp = BOOLEAN_TO_JSVAL((re->flags & JSREG_FOLD) != 0))
>+DEFINE_GETTER(MULTILINE, *vp = BOOLEAN_TO_JSVAL((re->flags & JSREG_MULTILINE) != 0))
>+DEFINE_GETTER(STICKY, *vp = BOOLEAN_TO_JSVAL((re->flags & JSREG_STICKY) != 0))

You can do without the GETTER thing here, so just

  >+#define DEFINE_GETTER(name, code)                                     \
  >+    static JSBool                                                     \
  >+    name(JSContext *cx, JSObject *obj, jsval id, jsval *vp) {         \
  [...]
  >+DEFINE_GETTER(regexp_lastIndex_getter, re = re; *vp = obj->getRegExpLastIndex())
  >+DEFINE_GETTER(regexp_source_getter, *vp = STRING_TO_JSVAL(re->source))
  [...]
  >+    {"source",     REGEXP_SOURCE,      RO_REGEXP_PROP_ATTRS,regexp_source_getter,NULL},
  [...]

>+DEFINE_GETTER(LAST_INDEX, re = re; *vp = obj->getRegExpLastIndex())

What's this "re = re;" for?

>+#define SETTER(prop) \
>+    prop##Setter
>+
> static JSBool
>-regexp_getProperty(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
>+SETTER(LAST_INDEX)(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {

Same thing here, but even more so since it's only used in one place.

>+#define STATIC_GETTER(prop) \
>+    prop##StaticGetter
>+
>+#define DEFINE_STATIC_GETTER(prop, code)                                     \
>+    static JSBool                                                            \
>+    STATIC_GETTER(prop)(JSContext *cx, JSObject *obj, jsval id, jsval *vp) { \
>+        JSRegExpStatics *res = &cx->regExpStatics;                           \
>+        { code; }                                                            \

Same thing here -- the ## magic doesn't seem necessary.

>+    {"input", 0, REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(INPUT), STATIC_SETTER(INPUT)},
>+    {"multiline", 0, REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(MULTILINE), STATIC_SETTER(MULTILINE)},
>+    {"lastMatch", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(LAST_MATCH), NULL},
>+    {"lastParen", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(LAST_PAREN), NULL},
>+    {"leftContext", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(LEFT_CONTEXT), NULL},
>+    {"rightContext", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(RIGHT_CONTEXT), NULL},
>+    {"$1", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(PAREN1), NULL},
>+    {"$2", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(PAREN2), NULL},
>+    {"$3", 0, RO_REGEXP_STATIC_PROP_ATTRS, STATIC_GETTER(PAREN3), NULL},
[...]

Line them up? Your call.

r=me with whatever changes you want to make.

Comment 3

[Andreas Gal :gal]

The re=re is necessary because I use a uniform code paths that always gets re here. In case of lastIndex we don't need it. This also means I lock the object now, which isn't necessary for that corner case either. However, its basically free for unshared regexps and we are about to eliminate locking anyway.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> The re=re is necessary because I use a uniform code paths that always gets re
> here. In case of lastIndex we don't need it.

You mean this avoids a warning? This could be handled with more macro factoring to define a getter that does no getPrivate at all for lastIndex.

Ugly macro reduction favors no ## pasting macros, or at least all-on-one-line definitions for them.

Great to see tinyid elimination on the move. Beware shortid is not the same thing though.

/be

Comment 5

[Brendan Eich [:brendan]]

+#define DEFINE_GETTER(prop, code)                                     \
+    static JSBool                                                     \
+    GETTER(prop)(JSContext *cx, JSObject *obj, jsval id, jsval *vp) { \

.12345678901234567890123456789012345678901234567890123456789012345678901234567890

Uniformity wants \ in column 99 -- it's worth it to reduce net ugliness a bit and help a macro-fairy get its wings.

Also, similarly: opening { on its own line.

/be

Comment 6

[Andreas Gal :gal]

We are about to eliminate object locking, so I don't think the macro re-factoring is worth it.

I fixed the { but column 99 looks horrible now because the macro is pretty lean. I moved it to column 70, even that is far away. I will fix in a follow-up if you insist.

Comment 7

[Brendan Eich [:brendan]]

Oops, I meant column 79! Sorry. Please use that in a followup. Don't invent a new columen 70 exception to the style rule, though.

/be

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 448311 [details] [diff] [review]
patch

Nice! thanks,

/be

Comment 11

[Andreas Gal :gal]

I did a little spot fix before checking in. For $1..$9 we have to check that index < parens.length().

Comment 12

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/3666268406e8

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3666268406e8