Closed Bug 1328948 Opened 3 years ago Closed 2 years ago

Add a checked cast method to Cell

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jonco, Assigned: allstars.chh)

Details

(Keywords: triage-deferred)

Attachments

(1 file, 1 obsolete file)

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.
Summary: Add a check cast method to Cell → Add a checked cast method to Cell
Keywords: triage-deferred
Priority: -- → P3
Assignee: nobody → allstars.chh
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8931576 - Flags: review?(jcoppeard)
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.
Attachment #8931576 - Flags: review?(jcoppeard) → review+
(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
Attached patch Patch. v2Splinter Review
This addressed item 1, return pointer instead in Jonco's previous comment.
Attachment #8931576 - Attachment is obsolete: true
Attachment #8932017 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/163e415d22df
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(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?
(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.
You need to log in before you can comment on or make changes to this bug.