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

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimb, Assigned: Waldo)

Tracking

unspecified
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
Posted 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)
Assignee

Comment 2

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=46dce5e2f958
Attachment #8338164 - Attachment is obsolete: true
Attachment #8338164 - Flags: review?(jimb)
Attachment #8338230 - Flags: review?(jimb)
Reporter

Comment 3

6 years ago
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...
Reporter

Comment 4

6 years ago
Hmm, I guess there really isn't any substance there to split out...
Assignee

Comment 5

6 years ago
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.
Reporter

Updated

6 years ago
Attachment #8338230 - Flags: review?(jimb) → review+
Reporter

Comment 6

6 years ago
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: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.