601709 - InstanceOf is an API that throws, not an API than merely tests (as one might expect)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

InstanceOf sounds like a function that simply returns a boolean indicating whether an object is an instance of a given class --- not like a function that throws an error if the object is not.

It would save code and improve clarity to:
- rename InstanceOf to RequireInstanceOf
- have it take a Value instead of a JSObject *, to avoid gratuitous calls to ComputeThisFromVp in functions that have no need of primitive wrapping, and
- have it return a JSObject * guaranteed to be NULL (on error) or an Object with the given class.

Perhaps something analogous could be done with JS_InstanceOf.

Comment 1

[Tom S [:evilpie]]

Attachment: Remove InstanceOf

Instead of renaming the function, i just removed it (and GetInstancePrivate). I think "if (!obj->isDate)" looks a lot cleaner than any function call. So we only have a few places left that use JSMSG_INCOMPATIBLE_PROTO/METHOD directly.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 524722 [details] [diff] [review]
Remove InstanceOf

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>@@ -2029,19 +2030,19 @@ JS_PUBLIC_DATA(Class) js_FunctionClass =
> 
> JSString *
> fun_toStringHelper(JSContext *cx, JSObject *obj, uintN indent)
> {
>     if (!obj->isFunction()) {
>         if (obj->isFunctionProxy())
>             return JSProxy::fun_toString(cx, obj, indent);
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
>-                             JSMSG_INCOMPATIBLE_PROTO,
>-                             js_Function_str, js_toString_str,
>-                             "object");
>+                         JSMSG_INCOMPATIBLE_PROTO,
>+                         js_Function_str, js_toString_str,
>+                         "object");
>         return NULL;

This realignment is wrong.  I understand the whitespace changes were made by your editor -- if it's making a change like this, you're going to have to disable the auto-correction feature, or at least carefully review the changes it makes.

Speaking of whitespace changes, there are a gazillion of them here that are completely unrelated to the change being made.  I'm all fine with whitespace corrections, but unless it's within the context of the desirable changes and isn't completely huge, I'd really rather not have to disentangle null changes from substantive changes, and neither does any other reviewer.

Anyway, please fix the whitespace over-fix above, and we can go ahead with this patch as-is since it's already done, and almost entirely acceptable.  But in the future, keep whitespace changes, unless directly in-context with intended changes, to entirely separate patches.  It's easy to review whitespace-only patches, much harder to review mixtures.

Comment 3

[Jeff Walden [:Waldo]]

Actually, I'll just fix that locally and push it with other patches tomorrow -- easy for me to fix, less hassle for everyone involved.

Comment 4

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/43cef42964d7
http://hg.mozilla.org/tracemonkey/rev/19df46c13176
http://hg.mozilla.org/tracemonkey/rev/6e7085e36bd4

Comment 5

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

http://hg.mozilla.org/mozilla-central/rev/43cef42964d7
http://hg.mozilla.org/mozilla-central/rev/19df46c13176
http://hg.mozilla.org/mozilla-central/rev/6e7085e36bd4