Closed
Bug 1328948
Opened 7 years ago
Closed 7 years ago
Add a checked cast method to Cell
Categories
(Core :: JavaScript: GC, defect, P3)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: jonco, Assigned: allstars.chh)
Details
(Keywords: triage-deferred)
Attachments
(1 file, 1 obsolete file)
5.22 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•7 years ago
|
Summary: Add a check cast method to Cell → Add a checked cast method to Cell
Updated•7 years ago
|
Keywords: triage-deferred
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → allstars.chh
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8931576 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
(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
Assignee | ||
Comment 4•7 years ago
|
||
This addressed item 1, return pointer instead in Jonco's previous comment.
Attachment #8931576 -
Attachment is obsolete: true
Attachment #8932017 -
Flags: review+
Pushed by yhuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/163e415d22df add is(), as() to Cell. r=jonco
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/163e415d22df
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Reporter | ||
Comment 7•7 years ago
|
||
(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?
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Description
•