Replace AutoShapeVector with Rooted<ShapeVector>

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(5 attachments)

Assignee

Description

4 years ago
This was eminently straightforward. I split this up more than I probably needed to, but it should at least help raise awareness of the new tools.
Attachment #8637447 - Flags: review?(jdemooij)
Assignee

Comment 1

4 years ago
And the ShapeVectors in self-hosting.
Attachment #8637448 - Flags: review?(till)
Assignee

Comment 2

4 years ago
Attachment #8637449 - Flags: review?(efaustbmo)
Assignee

Comment 3

4 years ago
Attachment #8637451 - Flags: review?(jorendorff)
Assignee

Comment 4

4 years ago
And those should be all the users.
Attachment #8637452 - Flags: review?(sphink)
Comment on attachment 8637448 [details] [diff] [review]
use_rooted_for_selfhosting_shapevec-v0.diff

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

r=me
Attachment #8637448 - Flags: review?(till) → review+
Comment on attachment 8637449 [details] [diff] [review]
use_rooted_for_scopeobj_shapevec-v0.diff

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

APPROVED.

::: js/src/vm/ScopeObject.cpp
@@ +850,5 @@
>  
>      for (Shape::Range<NoGC> r(srcBlock->lastProperty()); !r.empty(); r.popFront())
> +        shapes[srcBlock->shapeToIndex(r.front())] = &r.front();
> +
> +    RootedId id(cx);

I like how you guys come through all the time and are like "people don't understand how to use Rooted at all" and just fix it. Thanks :)
Attachment #8637449 - Flags: review?(efaustbmo) → review+
Comment on attachment 8637447 [details] [diff] [review]
use_rooted_for_baseline_shapevec-v0.diff

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

What about RootedShapeVector/HandleShapeVector typedefs for consistency, or do you think that's confusing?
Attachment #8637447 - Flags: review?(jdemooij) → review+
Assignee

Comment 8

4 years ago
(In reply to Jan de Mooij [:jandem] from comment #7)
> Comment on attachment 8637447 [details] [diff] [review]
> use_rooted_for_baseline_shapevec-v0.diff
> 
> Review of attachment 8637447 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about RootedShapeVector/HandleShapeVector typedefs for consistency, or
> do you think that's confusing?

I recently noticed that someone typedefed RootedScriptSource to Rooted<ScriptSourceObject>. Of course this was only after a frantic hour of trying to figure out how in the world Rooted<ScriptSource> could possibly have compiled and what GC bugs it had introduced. This sort of thing makes me think that the typedefs were a mistake.
Comment on attachment 8637452 [details] [diff] [review]
remove_AutoShapeVector-v0.diff

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

\o/
Attachment #8637452 - Flags: review?(sphink) → review+
Assignee

Updated

4 years ago
Keywords: leave-open
Assignee

Updated

4 years ago
Attachment #8637447 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8637448 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8637449 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8637451 - Flags: review?(jorendorff) → review?(jcoppeard)
Attachment #8637451 - Flags: review?(jcoppeard) → review+
Assignee

Updated

4 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/6cc0bc6b707d
https://hg.mozilla.org/mozilla-central/rev/7cbb4de9b9c6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.