Closed Bug 940033 Opened 7 years ago Closed 7 years ago
Map Entry::key should be non-const, but private, with read-only accessor
js::HashMapEntry::key should be a private member with a 'const Key &' accessor, not a public const member. It is essential to js::HashMap's correctness that an entry's key not be changed without it being rehashed, so HashMapEntry::key is const. However, for the move constructor and move assignment operator on HashMapEntry, the key needs to be non-const. If we made 'key' private, and provided only a 'const Key &' accessor, then we could have confidence after local inspection that it was only being changed at appropriate points, while still allowing the move constructor and move assignment operator to be written without using const_cast.
Lots of s/(\.|->)(key|value)/\1\2\(\)/g here, largely manually and/or with the help of an editor s&r on files flagged by compilation attempts, a few places where I added extra temporaries to slightly reduce the CSE craziness. JS engine builds, browser builds (both debug, JS also completed with exact rooting/generational). What a pain.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #8338164 - Flags: review?(jimb)
Wow, that must have been grueling. Thanks for doing this! It's easier to review if the mechanical changes are put in their own patch. But if you survived, I will too...
Hmm, I guess there really isn't any substance there to split out...
I didn't really do this mechanically, actually, at all. Just started with HashTable.h and kept moving through the JS engine compile errors til they were all gone, then MXR'd for outside of js/src, then a few extra fixes from try. Maybe there's a tool to do this, but I'm not aware of/familiar with it.
Attachment #8338230 - Flags: review?(jimb) → review+
Google has one that they use internally. I looked for it a little bit, but didn't find it.
Target Milestone: --- → mozilla28
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.