Jason Orendorff [:jorendorff] Reporter
	Description • 11 years ago

jsobj.h defines a few dozen inline methods JSObject::isDate(), isFunction(), and so on.

This is kind of annoying because they're defined in jsobjinlines.h. The reason for that is that they refer to class objects that... well, it's a long story.

Anyway, I bet we could have somewhat less #include "jsobjinlines.h" if we had member function templates is<T>() and as<T>() to class JSObject.

	Nicholas Nethercote [inactive] Assignee
	Comment 3 • 11 years ago

Oh crap, yesterday I was working on a patch that simply moved most of those isFoo() function definitions from jsobjinlines.h to jsobj.h.  This let me remove |#include "jsobjinlines.h"| from quite a few files -- I had to move some other stuff around, of course -- and I got the number of files included during compilation down from ~135,000 to ~120,000.

So, we're stepping on each others' toes here.  How do you want to proceed?

	Jason Orendorff [:jorendorff] Reporter
	Comment 4 • 11 years ago

It's all yours!

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 11 years ago

Actually, I'll do that in bug 880565.  This template thing is a good idea for a follow-up.

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 11 years ago

This is just jorendorff's previous Part 1 patch.  r=me.

	Nicholas Nethercote [inactive] Assignee
	Comment 7 • 11 years ago

The only tricky part here is that is<ArgumentsObject> needs to be specialized,
because ArgumentsObject doesn't have its own Class.

	Nicholas Nethercote [inactive] Assignee
	Comment 12 • 11 years ago

(Fixed the part number in the commit message.)

	Nicholas Nethercote [inactive] Assignee
	Comment 19 • 11 years ago

The only tricky bit here is that I removed CallClass from the friend API and
replaced it with IsCallObject(), which is enough to satisfy the one consumer of
CallClass in Gecko.

	Nicholas Nethercote [inactive] Assignee
	Comment 20 • 11 years ago

DeclEnvClass was in the friend API, but was unused in Gecko.

	Nicholas Nethercote [inactive] Assignee
	Comment 21 • 11 years ago

The only tricky bit here is that is<NestedScopeObject>() needs to be specialized.

	Luke Wagner [:luke]
	Comment 22 • 11 years ago

Neat, this looks much nicer.  I had run into the problem that you can't forward declare static member variables, but it seems like the late type checking of template functions is/as avoids this problem.  Sweet!

	Tom S [:evilpie]
	Comment 23 • 11 years ago

Comment on attachment 762548 [details] [diff] [review]
(part 13) - Use JSObject::{is,as} for CallObject.

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

::: js/src/jsfriendapi.h
@@ -375,5 @@
>  };
>  
>  } /* namespace shadow */
>  
> -extern JS_FRIEND_DATA(js::Class) CallClass;

neat

	Tom S [:evilpie]
	Comment 24 • 11 years ago

Comment on attachment 762550 [details] [diff] [review]
(part 15) - Use JSObject::{is,as} for NestedScopeObject.

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

I would rather not add any magic for one use. I believe it makes more sense to specialize this locally for the two cases.

	Nicholas Nethercote [inactive] Assignee
	Comment 25 • 11 years ago

> I would rather not add any magic for one use. I believe it makes more sense
> to specialize this locally for the two cases.

That's a fair point.  However, vm/ScopeObject.h has this class hierarchy in a nice comment:

 *   JSObject                   Generic object
 *     \
 *   ScopeObject                Engine-internal scope
 *     \   \   \
 *      \   \  DeclEnvObject    Holds name of recursive/heavyweight named lambda
 *       \   \
 *        \  CallObject         Scope of entire function or strict eval
 *         \
 *   NestedScopeObject          Scope created for a statement
 *     \   \
 *      \  WithObject           with
 *       \
 *   BlockObject                Shared interface of cloned/static block objects
 *     \   \
 *      \  ClonedBlockObject    let, switch, catch, for
 *       \
 *       StaticBlockObject      See NB

and I think it's reasonable to want to have an is<> function for every object in this hierarchy (and indeed for every kind of special object).

Also, in patch 2 there's a similar specialization for is<ArgumentsObject>(), which just does |is<StrictArgumentsObject>() || is<NormalArgumentsObject>()|.

Does that change your opinion?

	Nicholas Nethercote [inactive] Assignee
	Comment 26 • 11 years ago

Parts 1--14:

https://hg.mozilla.org/integration/mozilla-inbound/rev/55db3dd4779b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a741a5faa4d3 
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b9f61b1d406 
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c5c0ae167d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ad3842177c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5ff76c69ae 
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5799292b8de 
https://hg.mozilla.org/integration/mozilla-inbound/rev/584ad9e66195 
https://hg.mozilla.org/integration/mozilla-inbound/rev/74831022f42f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/45f4120fe067 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c473465f95b8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a2fb737fea 
https://hg.mozilla.org/integration/mozilla-inbound/rev/af5e07a1308b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b688861270 

(The try run was at https://tbpl.mozilla.org/?tree=Try&rev=77c5a98b9203.)

	Nicholas Nethercote [inactive] Assignee
	Comment 28 • 11 years ago

This one was disappointing.  Because JSObject::getProto() is in jsobjinlines.h,
is<ClonedBlockObject>() and is<StaticBlockObject> both ended up in
ScopeObject-inl.h rather than ScopeObject.h.

	Nicholas Nethercote [inactive] Assignee
	Comment 29 • 11 years ago

The only unusual part of this is that JSObject::enclosingScope() moved from
jsobjinlines.h to vm/ScopeObject-inl.h, and ScopeObject::enclosingScope() moved
from vm/ScopeObject-inl.h to vm/ScopeObject.h.

	Tom S [:evilpie]
	Comment 33 • 11 years ago

Comment on attachment 762550 [details] [diff] [review]
(part 15) - Use JSObject::{is,as} for NestedScopeObject.

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

I don't really have a much better idea, so let's roll with it.

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/55db3dd4779b
https://hg.mozilla.org/mozilla-central/rev/a741a5faa4d3
https://hg.mozilla.org/mozilla-central/rev/4b9f61b1d406
https://hg.mozilla.org/mozilla-central/rev/23c5c0ae167d
https://hg.mozilla.org/mozilla-central/rev/a9ad3842177c
https://hg.mozilla.org/mozilla-central/rev/5a5ff76c69ae
https://hg.mozilla.org/mozilla-central/rev/f5799292b8de
https://hg.mozilla.org/mozilla-central/rev/584ad9e66195
https://hg.mozilla.org/mozilla-central/rev/74831022f42f
https://hg.mozilla.org/mozilla-central/rev/45f4120fe067
https://hg.mozilla.org/mozilla-central/rev/c473465f95b8
https://hg.mozilla.org/mozilla-central/rev/94a2fb737fea
https://hg.mozilla.org/mozilla-central/rev/af5e07a1308b
https://hg.mozilla.org/mozilla-central/rev/90b688861270

	Nicholas Nethercote [inactive] Assignee
	Comment 35 • 11 years ago

We already have |FunctionObject|, except it's called |JSFunction|.  I left that
unchanged because it occurs a bazillion times.

We also already have toFunction(), which I replaced with as<JSFunction>().

I changed js::FunctionClass to FunctionObject::class_, as usual.  But
FunctionClass was in the friend API, and in jsgcinlines.h we need to use it
without #including jsfun.h.  So I also added a
js::FunctionClassPtr value to the friend API which is equal to
FunctionObject::class_.  Ugly, but I can't see a better way out.  Suggestions
are welcome.

I also added js::IsFunctionObject() and used it to neaten up
nsScriptSecurityManager.cpp a bit.

In JS_GetTraceThingInfo there were two impossible cases: |!fun| and |fun!=obj|.
I removed them.

The rest of the patch is as tedious as you'd expect.  Sorry.

	Nicholas Nethercote [inactive] Assignee
	Comment 36 • 11 years ago

Parts 15--19:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f59d274b0ed2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26720cbf048
https://hg.mozilla.org/integration/mozilla-inbound/rev/b35dcd7b0985
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3cacf6ba53
https://hg.mozilla.org/integration/mozilla-inbound/rev/20542fdcbe71

	Ed Morley [:emorley]
	Comment 37 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f59d274b0ed2
https://hg.mozilla.org/mozilla-central/rev/d26720cbf048
https://hg.mozilla.org/mozilla-central/rev/b35dcd7b0985
https://hg.mozilla.org/mozilla-central/rev/0f3cacf6ba53
https://hg.mozilla.org/mozilla-central/rev/20542fdcbe71

	Steve Fink [:sfink] [:s:]
	Comment 38 • 11 years ago

Comment on attachment 763475 [details] [diff] [review]
(part 21) - Use JSObject::{is,as} for GlobalObject.

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

::: js/src/jsobj.cpp
@@ +1286,5 @@
>  void
>  NewObjectCache::fillProto(EntryIndex entry, Class *clasp, js::TaggedProto proto,
>                            gc::AllocKind kind, JSObject *obj)
>  {
> +    JS_ASSERT_IF(proto.isObject(), !proto.toObject()->is<GlobalObject>());

*Totally* unrelated to this patch, but the toObject()-> threw me for a loop. Why does TaggedProto have a toObject() that returns a JSObject* instead of a JSObject& like Value::toObject()? It even has a separate toObjectOrNull()! (You don't have to answer this question.)

	Terrence Cole [:terrence]
	Comment 39 • 11 years ago

Presumably there isn't any untagged representation sitting around in memory anywhere to make the ref to, so it has to return a copy with the tag bits removed.

	Steve Fink [:sfink] [:s:]
	Comment 40 • 11 years ago

Comment on attachment 764004 [details] [diff] [review]
(part 22) - Use JSObject::{is,as} for JSFunction.

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

::: js/src/jsapi-tests/testOriginPrincipals.cpp
@@ +86,5 @@
>  {
>      JS::RootedValue rval(cx);
>      CHECK(eval(asciiChars, principal, originPrincipal, rval.address()));
>  
> +    JSScript *script = JS_GetFunctionScript(cx, &JSVAL_TO_OBJECT(rval)->as<JSFunction>());

I know it's a different cleanup, but can you do rval.toObject().as<JSFunction>() here?

::: js/src/jsapi.cpp
@@ -2642,5 @@
> -                if (!fun) {
> -                    JS_snprintf(buf, bufsize, " <newborn>");
> -                } else if (fun != obj) {
> -                    JS_snprintf(buf, bufsize, " %p", fun);
> -                } else {

nice

::: js/src/jsfun.cpp
@@ +508,5 @@
>      NULL,                    /* construct   */
>      fun_trace
>  };
>  
> +JS_FRIEND_DATA(Class*) js::FunctionClassPtr = &JSFunction::class_;

I can't think of anything better for this. The only other option I can think of is

  extern JS_FRIEND_API(Class*) js::FunctionClass();

(I like your name FunctionClassPtr better for a data member. Not sure it needs the Ptr suffix for a function return value.)

Up to you.

::: js/src/jsscriptinlines.h
@@ +96,5 @@
>  inline JSFunction *
>  JSScript::getFunction(size_t index)
>  {
>      JSObject *funobj = getObject(index);
> +    JSFunction *fun = &funobj->as<JSFunction>();

I'd merge these into one line now; the funobj temporary doesn't add much.

::: js/src/jsstr.cpp
@@ +42,5 @@
>  #include "vm/GlobalObject.h"
>  #include "vm/Interpreter.h"
>  #include "vm/NumericConversions.h"
>  #include "vm/RegExpObject.h"
> +#include "vm/ScopeObject.h"

why?

	Nicholas Nethercote [inactive] Assignee
	Comment 41 • 11 years ago

> > +JS_FRIEND_DATA(Class*) js::FunctionClassPtr = &JSFunction::class_;
> 
> I can't think of anything better for this. The only other option I can think
> of is
> 
>   extern JS_FRIEND_API(Class*) js::FunctionClass();

I ended up sticking with FunctionClassPtr, because it's used in jsfriendapi.h and I don't think a function would have worked.  However, I did realize it doesn't need to be JS_FRIEND_DATA (thanks to js::IsFunctionObject()), so I removed that.


> > +#include "vm/ScopeObject.h"
> 
> why?

Um, not sure.  I've taken it out.

	Nicholas Nethercote [inactive] Assignee
	Comment 42 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6da87883494a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8526023eb2b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c6097e5c4d4


> However, I did realize it doesn't need to be JS_FRIEND_DATA (thanks to
> js::IsFunctionObject()), so I removed that.

The Windows build dissented, so I put it back in before landing.

	Ed Morley [:emorley]
	Comment 43 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6da87883494a
https://hg.mozilla.org/mozilla-central/rev/8526023eb2b1
https://hg.mozilla.org/mozilla-central/rev/1c6097e5c4d4