Closed
Bug 761457
Opened 12 years ago
Closed 12 years ago
Make NonGenericMethodGuard's signature more normal
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: Waldo, Assigned: Waldo)
Details
Attachments
(1 file)
46.50 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Right now NonGenericMethodGuard's signature is like this: JSObject* NonGenericMethodGuard(..., Class* clasp, bool* ok); And the dance you do to properly call it looks basically like this: bool ok; JSObject* thisObj = NonGenericMethodGuard(..., clasp, &ok); if (!thisObj) return ok; // thisObj has clasp, carry on The normal case is that a method returns NULL to indicate failure, outside the rare infallible method. But this method's fallible! So this pattern violates that idiom. There's never going to be anything very nicely natural here, I think. Maybe an object with class methods on it might be truly nice, but I couldn't think of a good, clean way to do it. In the meantime, we should at least get rid of the failure-looking return that isn't. Let's make the signature this instead: bool NonGenericMethodGuard(..., Class* clasp, JSObject** thisObj); and the normal dance this: JSObject* thisObj; if (!NonGenericMethodGuard(..., clasp, &thisObj)) return false; if (!thisObj) return true; // thisObj has clasp, carry on The last condition's still a bit weird, to be sure. But it does communicate failure the normal way, which is a big help in using it.
Attachment #630011 -
Flags: review?(luke)
Comment 1•12 years ago
|
||
Comment on attachment 630011 [details] [diff] [review] Patch Thanks for fixing this up; it does seem more regular this way. >diff --git a/js/src/vm/MethodGuard.h b/js/src/vm/MethodGuard.h >+ * 1. NonGenericMethodGuard returned false because it encountered an error: >+ * args.thisv wasn't an object, was an object of the wrong class, was a >+ * proxy to an object of the wrong class, or was a proxy to an object of >+ * the right class but the recursive call to args.callee failed. This ... can you take out the double spaces after .?
Attachment #630011 -
Flags: review?(luke) → review+
Assignee | ||
Comment 2•12 years ago
|
||
Haters gonna hate. I also had to make some minor adjustments to BoxedPrimitiveMethodGuard, sniffed out by a try run. https://hg.mozilla.org/integration/mozilla-inbound/rev/7d12c871a707
Target Milestone: --- → mozilla16
Comment 3•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7d12c871a707
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 4•12 years ago
|
||
The new dance is even more tangled with rooting, btw. This goes from bool ok; RootedObject thisObj(NonGenericMethodGuard(..., clasp, &ok)); if (!thisObj) return ok; ... to JSObject* thisObjUnrooted; if (!NonGenericMethodGuard(..., clasp, &thisObj)) return false; if (!thisObj) return true; RootedObject thisObj(thisObjUnrooted); ... which makes me want something more like NonGenericMethodGuard guard(cx, args, native, clasp); if (guard.handled()) return guard.status(); RootedObject thisObj(guard.thisObj()); ...
Comment 5•12 years ago
|
||
When I merged this to IM I had to fix the ParallelArray uses and I noticed you can also write it like this (just one line shorter): RootedObject thisObjUnrooted(cx); if (!NonGenericMethodGuard(..., clasp, thisObj.address())) return false; if (!thisObj) return true;
Comment 6•12 years ago
|
||
Oh, you're right. That's far more tolerable. I can live with that. Though I still prefer a different interface, mostly because I don't like the double-check. The previous version felt totally bizarre because the interface was returning two values with unfamiliar semantics, but once I figured it out it actually felt pretty good. Probably because my natural desire was to think of it as "do the wacky proxy stuff. Ok, did that handle it for me? Then return. Otherwise, go do the stuff." So I kind of want an API along those lines instead of "did the proxy gunk handle this unsuccessfully? How about successfully? No to both? Ok, go do the stuff."
You need to log in
before you can comment on or make changes to this bug.
Description
•