771018 - Stop using "const Shape" all over the place

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

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.

Comment 1

[Brian Hackett [Laid off!]]

Yay!  Agreed about the fundamental meaninglessness of 'const Shape'.

Comment 2

[Luke Wagner [:luke]]

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.

Comment 3

[Luke Wagner [:luke]]

Actually, to highlight the point you made in comment 0, second para, why don't we annotate all Shape* with 'volatile'?

Comment 4

[Brian Hackett [Laid off!]]

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

[Luke Wagner [:luke]]

Hehe, it looks like I wasn't ridiculous enough.  I should have used <sarcasm> tags ;)

Comment 6

[Jeff Walden [:Waldo]]

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

[Luke Wagner [:luke]]

Oh boy, another bug 752223.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/mozilla-central/rev/fbd96a0bcc00