1328948 - Add a checked cast method to Cell

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P3)

Description

[Jon Coppeard (:jonco)]

We should add a method to Cell so that you can do:

  cell->as<JSOjbect*>() 

And it will do the cast for you and assert that the type matches the thing kind.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8931576 [details] [diff] [review]
Patch.

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

This is great.  Thanks for doing this.

Two possible improvements:

Since we always seem to end up doing &as() in the callers, we should probably make the as() methods return pointers rather than references.

Secondly, for maximum efficiency we should also add versions of these methods to TenuredCell since the getTraceKind() method there doesn't have to check whether the cell is in the nursery.

But this is actually fine to land as it is.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #2)
> Secondly, for maximum efficiency we should also add versions of these
> methods to TenuredCell since the getTraceKind() method there doesn't have to
> check whether the cell is in the nursery.
Jonco, thanks for your review, but I am not quite sure what you meant for item 2.

Do you mean for the is() method I added, 

    template <class T>
    inline bool is() const {
        return getTraceKind() == JS::MapTypeToTraceKind<T>::kind;
    }

It will call getTraceKind, and getTraceKind will check if the cell is Tentured or not,
but I don't get how does this relate to add another 'as()' method to TenuredCell?

Can you explain more on this?
I will address this in a new bug.

Thanks

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

This addressed item 1, return pointer instead in Jonco's previous comment.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/163e415d22df
add is(), as() to Cell. r=jonco

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/163e415d22df

Comment 7

[Jon Coppeard (:jonco)]

(In reply to Yoshi Huang [:allstars.chh] from comment #3)
TenuredCell is derived from Cell so if you call is() and as() on a TenuredCell it will call the versions in Cell.  Cell::is() will call Cell::getTraceKind(), which will check whether the cell is tenured.  If it's a TenuredCell we already know that it is.  So really we want to add a TenuredCell::is() that's the same as Cell::is().  However, since it's in TenuredCell it will call TenuredCell::getTraceKind() which will skip the tenured check.  We'll need to also add versions of as() to TenuredCell so they will call TenuredCell::is() too.

Does that make sense?

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jon Coppeard (:jonco) from comment #7)
> (In reply to Yoshi Huang [:allstars.chh] from comment #3)
> TenuredCell is derived from Cell so if you call is() and as() on a
> TenuredCell it will call the versions in Cell.  Cell::is() will call
> Cell::getTraceKind(), which will check whether the cell is tenured.  If it's
> a TenuredCell we already know that it is.  So really we want to add a
> TenuredCell::is() that's the same as Cell::is().  However, since it's in
> TenuredCell it will call TenuredCell::getTraceKind() which will skip the
> tenured check.  We'll need to also add versions of as() to TenuredCell so
> they will call TenuredCell::is() too.
> 
> Does that make sense?

Yes, thanks for explanation.
I filed bug 1421152 for this.