Closed Bug 880041 Opened 7 years ago Closed 7 years ago

Add JSObject::is<T>() member function template

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jorendorff, Assigned: njn)

References

Details

Attachments

(22 files, 3 obsolete files)

10.31 KB, patch
njn
: review+
Details | Diff | Splinter Review
35.60 KB, patch
luke
: review+
Details | Diff | Splinter Review
26.97 KB, patch
luke
: review+
Details | Diff | Splinter Review
43.17 KB, patch
luke
: review+
Details | Diff | Splinter Review
6.72 KB, patch
sfink
: review+
Details | Diff | Splinter Review
8.36 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.66 KB, patch
sfink
: review+
Details | Diff | Splinter Review
16.47 KB, patch
sfink
: review+
Details | Diff | Splinter Review
28.41 KB, patch
sfink
: review+
Details | Diff | Splinter Review
6.99 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
8.00 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
18.50 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
47.75 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
17.72 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
7.33 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
24.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
33.60 KB, patch
luke
: review+
Details | Diff | Splinter Review
40.16 KB, patch
luke
: review+
Details | Diff | Splinter Review
35.89 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
6.60 KB, patch
sfink
: review+
Details | Diff | Splinter Review
57.74 KB, patch
sfink
: review+
Details | Diff | Splinter Review
159.82 KB, patch
sfink
: review+
Details | Diff | Splinter Review
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.
Attached patch Part 1, WIP 1 (obsolete) — Splinter Review
Assignee: general → jorendorff
Attached patch Part 2 - MapObject, WIP 1 (obsolete) — Splinter Review
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?
Blocks: 880088
It's all yours!
Summary: Add JSObject::is<T>() member function template → Partially untangle jsobjinlines.h
Assignee: jorendorff → n.nethercote
Attachment #758863 - Attachment is obsolete: true
Summary: Partially untangle jsobjinlines.h → Add JSObject::is<T>() member function template
Actually, I'll do that in bug 880565.  This template thing is a good idea for a follow-up.
This is just jorendorff's previous Part 1 patch.  r=me.
Attachment #762520 - Flags: review+
Attachment #758864 - Attachment is obsolete: true
The only tricky part here is that is<ArgumentsObject> needs to be specialized,
because ArgumentsObject doesn't have its own Class.
Attachment #762521 - Flags: review?(luke)
(Fixed the part number in the commit message.)
Attachment #762528 - Flags: review?(sphink)
Attachment #762527 - Attachment is obsolete: true
Attachment #762527 - Flags: review?(sphink)
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.
Attachment #762548 - Flags: review?(evilpies)
DeclEnvClass was in the friend API, but was unused in Gecko.
Attachment #762549 - Flags: review?(evilpies)
The only tricky bit here is that is<NestedScopeObject>() needs to be specialized.
Attachment #762550 - Flags: review?(evilpies)
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!
Attachment #762521 - Flags: review?(luke) → review+
Attachment #762524 - Flags: review?(luke) → review+
Attachment #762525 - Flags: review?(luke) → review+
Attachment #762529 - Flags: review?(sphink) → review+
Attachment #762526 - Flags: review?(sphink) → review+
Attachment #762528 - Flags: review?(sphink) → review+
Attachment #762541 - Flags: review?(sphink) → review+
Attachment #762542 - Flags: review?(sphink) → review+
Attachment #762544 - Flags: review?(evilpies) → review+
Attachment #762545 - Flags: review?(evilpies) → review+
Attachment #762546 - Flags: review?(evilpies) → review+
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
Attachment #762548 - Flags: review?(evilpies) → review+
Attachment #762549 - Flags: review?(evilpies) → review+
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.
Attachment #762550 - Flags: review?(evilpies)
> 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?
Depends on: 883696
No longer depends on: 883696
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.
Attachment #763462 - Flags: review?(luke)
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.
Attachment #763463 - Flags: review?(luke)
Attachment #763399 - Flags: review?(luke) → review+
Attachment #763462 - Flags: review?(luke) → review+
Attachment #763463 - Flags: review?(luke) → review+
Depends on: 883466
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.
Attachment #762550 - Flags: review+
Attachment #763471 - Flags: review?(evilpies) → review+
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.
Attachment #764004 - Flags: review?(sphink)
Attachment #763473 - Flags: review?(sphink) → review+
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.)
Attachment #763475 - Flags: review?(sphink) → review+
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.
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?
Attachment #764004 - Flags: review?(sphink) → review+
> > +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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.