Bug 164137 – [DOM] Introduce WebCore::jsDynamicDowncast

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

It is thin wrapper for jsDynamicCast. But it selects the faster implementation if the given target type is WebCore extending JSType.

[Yusuke Suzuki]

Rename it to jsDynamicDowncast.

[Yusuke Suzuki]

Created attachment 293189 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 293209 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 293215 [details]
Patch

[Yusuke Suzuki]

Created attachment 293227 [details]
Patch

[Yusuke Suzuki]

Created attachment 293242 [details]
Patch

[Darin Adler]

Comment on attachment 293242 [details]
Patch

Seems OK like this.

Another idea is to pattern this more on the WebCore is<> and downcast<> macros. The style where we return a nullptr when we can’t cast is indeed how dynamic_cast works, but it might not be as efficient as separate boolean checks and it results in working with pointers when we could be working with references instead.

[Yusuke Suzuki]

(In reply to comment #7)
> Comment on attachment 293242 [details]
> Patch
> 
> Seems OK like this.
> 
> Another idea is to pattern this more on the WebCore is<> and downcast<>
> macros. The style where we return a nullptr when we can’t cast is indeed how
> dynamic_cast works, but it might not be as efficient as separate boolean
> checks and it results in working with pointers when we could be working with
> references instead.

Interesting. Currently, we just reuse the semantics of C++ dynamic_cast.
And I think it is ok since typical JSC objects are held in JSC::JSValue or pointers.
But if converting pointer to reference for JSC objects in WebCore becomes more common,
I think it is worth introducing this is<> & downcast<> idiom.

[Yusuke Suzuki]

Before landing, I'll slightly extend the above patch to accept const pointer.
Currently, no one uses this. But it should be since const pointer will select the slow implementation.

[Darin Adler]

I don’t think that is<> and downcast<> was invented just so we can use it with references. We thought it was a clearer idiom at the call site than dynamic_cast for many uses and equally efficient.

[Yusuke Suzuki]

(In reply to comment #10)
> I don’t think that is<> and downcast<> was invented just so we can use it
> with references. We thought it was a clearer idiom at the call site than
> dynamic_cast for many uses and equally efficient.

Interesting. Is there any notes about the pros of is<> and downcast<> over dynamic_cast<> thing?
We frequently use the dynamic casting idiom like,

if (auto* xxx = jsDynamicCast<XXX*>(pointer)) {
    // do something on xxx.
}

[Darin Adler]

One difference with the is<> and downcast<> is that you do have to state the type twice, unlike with dynamic_cast. But I suppose the other difference is that you don't end up with a pointer that you know is non-null, so maybe it is particularly good because we like to use references for those kinds of things. I understand that's not so popular with the JavaScript people and I think Phil even wrote an opinion piece about why it's wrong to ever use a reference to an object stored in a garbage collected cell. Didn't convince me, but I did read it.

What downcast<> is particularly good for is the case where you have some other reason to know that is<> is true; you still get an ASSERT-checked cast in that case.

And is<> is good for cases where you want to make a type check, but not to guard use of a pointer of the more specific type.

One good feature of is<> and downcast<> that dynamic_cast<> does not have, but I suppose jsDynamicCast<> could have, is a compile time error when the cast is used unnecessarily on a pointer or reference that already has the appropriate type.

As you alluded to earlier, since is<> and downcast<> work on references, they can omit unnecessary null checks that the compiler might not be able to optimize out from a use of dynamic_cast.

The terms “is” and “downcast” seem slightly more direct than “dynamic_cast”. The “dynamic” in “dynamic_cast” could mean more than one thing, although I suppose C++ programmers need to learn what it means.

I can’t think of any other pros and cons off hand. Maybe Chris, creator of is<> and downcast<> can think of some.

[Yusuke Suzuki]

Thank you for your clarification!

(In reply to comment #12)
> One difference with the is<> and downcast<> is that you do have to state the
> type twice, unlike with dynamic_cast.

Hmm, to me, it looks a bit dangerous.

When using is<> and downcast<>, instead of

if (auto* xxx = jsDynamicCast<XXX*>(...)) {
    // do something on xxx.
}

we need to write,

if (is<XXX>(...)) {
    auto* xxx = downcast<XXX>(...);
    // do something on xxx.
}

At that time, we need to write XXX twice. If we accidentally write (like, substituting one thing, but forget substituting the other) the different things,
the compiler does not find it. Only the runtime assertion will catch this, correct?

if (is<XXX>(...)) {
    auto* xxx = downcast<YYY>(...);
    // do something on xxx.
}

> But I suppose the other difference is
> that you don't end up with a pointer that you know is non-null, so maybe it
> is particularly good because we like to use references for those kinds of
> things.

Yes, that is good thing. So I thought that is<> & downcast<> are useful if we like references.

> I understand that's not so popular with the JavaScript people and I
> think Phil even wrote an opinion piece about why it's wrong to ever use a
> reference to an object stored in a garbage collected cell. Didn't convince
> me, but I did read it.

OK, so I think is<> & downcast<> works nicely when using references.

> What downcast<> is particularly good for is the case where you have some
> other reason to know that is<> is true; you still get an ASSERT-checked cast
> in that case.

That is good. But jsCast<> also has the same thing.
We prefer to use jsCast<> in that case instead of using static_cast.

> And is<> is good for cases where you want to make a type check, but not to
> guard use of a pointer of the more specific type.

It's good thing. We typically use `cell->inherits(JSXXX::info())`.
Naming it as `jsIs<>` could be good.

> One good feature of is<> and downcast<> that dynamic_cast<> does not have,
> but I suppose jsDynamicCast<> could have, is a compile time error when the
> cast is used unnecessarily on a pointer or reference that already has the
> appropriate type.

Yeah. As you said, I think this feature can be easily introduced to jsDynamicCast<>.
And it is not the essential pros of is<> & downcast<> over jsDynamicCast<>, right?
BTW, introducing it to jsDynamicCast<> is nice. We can figure out some crazy things like jsDynamicCast<JSObject*>(jsObjectValue)...

> As you alluded to earlier, since is<> and downcast<> work on references,
> they can omit unnecessary null checks that the compiler might not be able to
> optimize out from a use of dynamic_cast.

Yes.

> The terms “is” and “downcast” seem slightly more direct than “dynamic_cast”.
> The “dynamic” in “dynamic_cast” could mean more than one thing, although I
> suppose C++ programmers need to learn what it means.

Yes.

> I can’t think of any other pros and cons off hand. Maybe Chris, creator of
> is<> and downcast<> can think of some.

That sounds nice.
Yes, if we use references, they are good. But if we handle pointers, I think dynamic_cast things are better since we do not write the type twice.
(Of course, this "dynamic_cast<>" is not the C++ default dynamic_cast<> thing. is<> & downcast<> are quite better than that. I mean it as `jsDynamicCast<>` thingy.)

[Darin Adler]

Chris, any other reason you chose this style for is<> and downcast<>?

[Yusuke Suzuki]

Created attachment 293339 [details]
Patch for landing. Extended for const pointers

[Yusuke Suzuki]

Comment on attachment 293339 [details]
Patch for landing. Extended for const pointers

Anyway, I'll land this patch since it effectively removes the special care in CodeGeneratorJS.pm :)
Once we figure out is<> and downcast<> are better, we will handle it in the separate patch.

[WebKit Commit Bot]

Comment on attachment 293339 [details]
Patch for landing. Extended for const pointers

Clearing flags on attachment: 293339

Committed r208124: <http://trac.webkit.org/changeset/208124>