Closed
Bug 940033
Opened 11 years ago
Closed 10 years ago
js::HashMapEntry::key should be non-const, but private, with read-only accessor
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: jimb, Assigned: Waldo)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
82.53 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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 | ||
Comment 2•11 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•10 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•10 years ago
|
||
Hmm, I guess there really isn't any substance there to split out...
Assignee | ||
Comment 5•10 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•10 years ago
|
Attachment #8338230 -
Flags: review?(jimb) → review+
Reporter | ||
Comment 6•10 years ago
|
||
Google has one that they use internally. I looked for it a little bit, but didn't find it.
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96d513cd89f
Target Milestone: --- → mozilla28
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b96d513cd89f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•