921081 - Templatize the NonGenericMethod helpers

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

We have a lot of

  static bool
  dosomething(JSContext *cx, unsigned argc, Value *vp)
  {
      CallArgs args = CallArgsFromVp(argc, vp);
      return CallNonGenericMethod<IsRegExp, dosomething_impl>(cx, args);
  }

boilerplate.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Templatize the NonGenericMethod helpers

With njn's is<FooObject> template function, it's possible to eliminate this entirely.

I haven't done all of the ones that could, but I thought it'd be a good time to check whether this is desireable or not (as in, now that I've burned more time than I should have...)

I also have to decide whether to make this a prereq of bug 861925, since I don't want to block that for very long on something trivial like this.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 810650 [details] [diff] [review]
Templatize the NonGenericMethod helpers

Review of attachment 810650 [details] [diff] [review]:
-----------------------------------------------------------------

Less typing of this stuff would be nice, yeah.  I think I worry somewhat about expanding linkage on so much stuff to do this, and what that does for code/linking efficiency.  Also, the typing needed to add this stuff in JSFunctionSpecs is maybe not the desirable amount.

So, um, I guess I'm lukewarm on the entire thing, or something.  Bleh.

::: js/src/jsobj.h
@@ +1519,5 @@
> +}
> +
> +template<typename ObjectType, bool Method(JSContext* cx, CallArgs args)>
> +bool
> +NonGenericMethod(JSContext* cx, unsigned int argc, JS::Value* vp)

gcc 4.4 and maybe 4.5 have had some difficulty, in my experience, with templated functions.  Or maybe with templated functions as members of classes.  Or, I dunno.  See dom/workers/Events.cpp and search for "struct Property" for the exact case hit.  It's workaroundable as there, but it's a little mess.  A try of b2g should expose any problems.

You should use JS::NativeImpl for the second parameter.

Comment 3

[Steve Fink [:sfink] [:s:]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Comment on attachment 810650 [details] [diff] [review]
> Templatize the NonGenericMethod helpers
> 
> Review of attachment 810650 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Less typing of this stuff would be nice, yeah.  I think I worry somewhat
> about expanding linkage on so much stuff to do this, and what that does for
> code/linking efficiency.

"Expanding linkage"? Not sure what that means?

The template expansions all happen in .cpp files, so I would think there'd be very little difference other than the mangled names being longer. 

> Also, the typing needed to add this stuff in
> JSFunctionSpecs is maybe not the desirable amount.

Well, you could always use macros.

    JS_NGFN("getTime", DateObject, DateObject::getTime, 0,0),

or

    JS_NONGEN("getTime", DateObject, DateObject::getTime, 0,0),

or

    JS_NONGENERIC("getTime", DateObject, DateObject::getTime, 0,0),

Let me also register my dislike for the term "nongeneric". I always have to think about that one for a while, and I'm still not sure I fully understand what it means. Can't it be "class specific" or something? "Class function"? "Class affiliated"? "Friendly" or "friend"?

Ooh! "Class native", or just "native" for short!!1!!1!

> 
> So, um, I guess I'm lukewarm on the entire thing, or something.  Bleh.

226 insertions(+), 564 deletions(-), and more to come. But yeah,

  (js::NonGenericMethod<RegExpObject, regexp_toString>)

isn't exactly purty either.

> ::: js/src/jsobj.h
> @@ +1519,5 @@
> > +}
> > +
> > +template<typename ObjectType, bool Method(JSContext* cx, CallArgs args)>
> > +bool
> > +NonGenericMethod(JSContext* cx, unsigned int argc, JS::Value* vp)
> 
> gcc 4.4 and maybe 4.5 have had some difficulty, in my experience, with
> templated functions.  Or maybe with templated functions as members of
> classes.  Or, I dunno.  See dom/workers/Events.cpp and search for "struct
> Property" for the exact case hit.  It's workaroundable as there, but it's a
> little mess.  A try of b2g should expose any problems.

Hm, that does look similar.

> You should use JS::NativeImpl for the second parameter.

Ah, right. I first encountered that type when doing this patch. It'll take me another month to internalize it.

Comment 4

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #3)
> Let me also register my dislike for the term "nongeneric".

It perhaps made more sense when you consider it was intended for use in implementing methods in ES5 which don't have "fully generic, doesn't have to have its |this| have a particular [[Class]]" verbiage in them.  But we've kind of made it more generic than that, such that it'll let you test for any sort of |this|, not just one class or so.  Updating the name somehow wouldn't hurt, but I don't really want to think about it.  :-)

Comment 5

[Steve Fink [:sfink] [:s:]]

I believe the intention now is to stop using the nongeneric method goop.