Last Comment Bug 771018 - Stop using "const Shape" all over the place
: Stop using "const Shape" all over the place
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-07-04 15:34 PDT by Bill McCloskey (:billm)
Modified: 2012-07-04 19:34 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (95.87 KB, patch)
2012-07-04 15:34 PDT, Bill McCloskey (:billm)
luke: review+
Details | Diff | Splinter Review

Description User image Bill McCloskey (:billm) 2012-07-04 15:34:29 PDT
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.
Comment 1 User image Brian Hackett (:bhackett) 2012-07-04 15:41:27 PDT
Yay!  Agreed about the fundamental meaninglessness of 'const Shape'.
Comment 2 User image Luke Wagner [:luke] 2012-07-04 15:48:44 PDT
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.
Comment 3 User image Luke Wagner [:luke] 2012-07-04 15:50:57 PDT
Actually, to highlight the point you made in comment 0, second para, why don't we annotate all Shape* with 'volatile'?
Comment 4 User image Brian Hackett (:bhackett) 2012-07-04 16:03:21 PDT
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 User image Luke Wagner [:luke] 2012-07-04 16:13:01 PDT
Hehe, it looks like I wasn't ridiculous enough.  I should have used <sarcasm> tags ;)
Comment 6 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-04 16:26:19 PDT
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 User image Luke Wagner [:luke] 2012-07-04 16:31:01 PDT
Oh boy, another bug 752223.
Comment 8 User image Bill McCloskey (:billm) 2012-07-04 19:34:55 PDT

Note You need to log in before you can comment on or make changes to this bug.