650931 - No API to determine if a JSObject is a RegExp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (no comments, no tests)

JS_NewRegExpObject (and such) return a JSObject, but there's no way to determine at a later point if that object is a RegExp. For dates we have JS_ObjectIsDate, functions JS_ObjectIsFunction.

Attached is a patch to make this work - no comments or tests though. If somebody wants to take this and run, go for it.

Comment 1

[David Mandelin [:dmandelin]]

I don't think we need comments or tests for something so small. Feel free to request review.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

That was easy... I'll have more of these for exposing other parts of a RegExp which should probably have tests (but that'll be a different bug so I'll bring it up when I file).

What's the process for small JS patches like this? Land in TM and just wait for a merge or land in m-c and it'll get merged to TM?

Comment 3

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> What's the process for small JS patches like this? Land in TM and just wait for
> a merge or land in m-c and it'll get merged to TM?

Either way is fine, so just do whichever is more convenient for you. You could also double-land if you want to land first on TM but get the change without waiting for the next merge.

Comment 4

[Jeff Walden [:Waldo]]

So, um, should this work like JS_IsArrayObject and also return true for wrapped regular expressions?  And I do think this needs a little bit of testing for corner-ish cases like that, at least if we're r+ing patches that are not considering such subtilties.

JS patches land in TM first as a general rule.

Comment 5

[Andreas Gal :gal]

RegExps don't work well across compartments. You are supposed to copy them, not wrap them.

Comment 6

[Philipp von Weitershausen [:philikon]]

Attachment: tests v1

Tests for JS_ObjectIsRegExp. Assuming r+ for those, is this good to go or do we need sr?

Comment 7

[Jeff Walden [:Waldo]]

I think that should be JS_IsRegExpObject for consistency with JS_IsArrayObject.  (Although, if that precedent weren't there, I'd say make it just JS_IsRegExp as "Object" is part of the method signature.)

Comment 8

[Andreas Gal :gal]

Comment on attachment 530972 [details] [diff] [review]
tests v1

This is a minor API change, I am confident Brendan is ok with it. No sr needed.

Comment 9

[Andreas Gal :gal]

I have no opinion on 7. JS_IsRegExpObject sounds pretty good too.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> I think that should be JS_IsRegExpObject for consistency with
> JS_IsArrayObject.

I was thinking that too, but then I saw there's also JS_ObjectIsFunction and JS_ObjectIsDate, so I didn't propose changing that. I really don't care either way.

Comment 11

[Jeff Walden [:Waldo]]

("consistency", said I?  Ha!)

IsFoo seems better to me because if you're asking the question, you probably have an object in mind, so the "subject" (as it were) of the inquiry can and should be implied.  (Example: in the JSON.parse Walk spec method, you first get a property from an object, then if it's an object you check whether it's an array or not and proceed accordingly.  The object to be inspected is your primary thought when you need to make the is-array check.)  Probably it doesn't matter overmuch, and if the name stays as-is, when jsapi2 comes we should be more careful to be consistent with the updated methods' names.

Comment 12

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/tracemonkey/rev/4b5ba020745e
http://hg.mozilla.org/tracemonkey/rev/adceec18aea0

Comment 13

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

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/4b5ba020745e
http://hg.mozilla.org/mozilla-central/rev/adceec18aea0