Stop using "const Shape" all over the place

RESOLVED FIXED in mozilla16



JavaScript Engine
5 years ago
5 years ago


(Reporter: billm, Assigned: billm)



Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
Created attachment 639192 [details] [diff] [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 2

5 years ago
Comment on attachment 639192 [details] [diff] [review]

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+

Comment 3

5 years ago
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.

Comment 5

5 years ago
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.)

Comment 7

5 years ago
Oh boy, another bug 752223.

Comment 8

5 years ago
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.