Closed Bug 771018 Opened 8 years ago Closed 8 years ago

Stop using "const Shape" all over the place

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file)

Attached patch patchSplinter Review
There are a lot of places where we annotate Shapes as being const, and a lot of other places where Shapes are not const. These annotations impose a cost: whenever we use Shape, we have to decide whether each particular instance should be const or non-const. Then sometimes we have to propagate the const annotation to other places, or add ugly casts that don't always make sense.

None of these annotations are really correct. Anytime we give an object a shape, we call setLastProperty and cast away the constness. This is necessary because objects make all sorts of changes to shapes, even the ones that are shared.

As a final argument, it seems unlikely to me that anyone has ever caught a real bug because we had a const annotation on a Shape.

So I propose we remove all the "const Shape"s at once so that we never have to think about them again.
Attachment #639192 - Flags: review?(luke)
Yay!  Agreed about the fundamental meaninglessness of 'const Shape'.
Comment on attachment 639192 [details] [diff] [review]
patch

Rock on.  I've almost been annoyed enough to do this on a few occasions but I was too lazy.
Attachment #639192 - Flags: review?(luke) → review+
Actually, to highlight the point you made in comment 0, second para, why don't we annotate all Shape* with 'volatile'?
I think using 'volatile' would just sow more confusion.  Isn't volatile intended for things that can be modified by the OS/hardware?  In this case all the writes will happen on the same thread, so Shape* rather than const Shape* or volatile Shape* seems correct.
Hehe, it looks like I wasn't ridiculous enough.  I should have used <sarcasm> tags ;)
No, you should have suggested waiting for Terrence's atomics work so that we could use AtomicPtr<Shape> for doubleplussafety.  (Had I been faster on the gun I would have suggested this before Brian could misinterpret it.)
Oh boy, another bug 752223.
https://hg.mozilla.org/mozilla-central/rev/fbd96a0bcc00
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.