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

RESOLVED FIXED in mozilla5

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jimb, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 621604
(Assignee)

Comment 1

8 years ago
Created attachment 524722 [details] [diff] [review]
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.
Attachment #524722 - Flags: review?(jwalden+bmo)

Updated

8 years ago
Attachment #524722 - Attachment is patch: true
Attachment #524722 - Attachment mime type: application/octet-stream → text/plain

Comment 2

8 years ago
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.
Attachment #524722 - Flags: review?(jwalden+bmo) → review+

Comment 3

8 years ago
Actually, I'll just fix that locally and push it with other patches tomorrow -- easy for me to fix, less hassle for everyone involved.
Assignee: general → evilpies
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Summary: InstanceOf should be renamed, take a Value, and return a JSObject * → InstanceOf is an API that throws, not an API than merely tests (as one might expect)
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.