Make NonGenericMethodGuard's signature more normal

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 630011 [details] [diff] [review]
Patch

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

5 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+
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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7d12c871a707
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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());
  ...
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;
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.