Closed Bug 940033 Opened 7 years ago Closed 7 years ago

js::HashMapEntry::key should be non-const, but private, with read-only accessor

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: jimb, Assigned: Waldo)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch (obsolete) — Splinter Review
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)
https://tbpl.mozilla.org/?tree=Try&rev=46dce5e2f958
Attachment #8338164 - Attachment is obsolete: true
Attachment #8338164 - Flags: review?(jimb)
Attachment #8338230 - 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.
https://hg.mozilla.org/mozilla-central/rev/b96d513cd89f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.