Don't expose HashTable entries directly from HashMap/HashSet

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 610296 [details] [diff] [review]
v0: and fix two bracket mis-alignments.

HashMap and HashSet only expose Entries through Ptr and Range, except for one variant of put.  This exception makes it somewhat more annoying to wrap HashMap and HashSet than it should be.  This exception also appears to be completely unused in practice, so I see nothing wrong with just making the output a bool.

With this gone, only Ptr and Range expose hash entries directly.
Attachment #610296 - Flags: review?(luke)

Updated

6 years ago
Attachment #610296 - Flags: review?(luke) → review+

Comment 1

6 years ago
Although, now that I think about it a little more, if you do the thing we talked about yesterday (created a new map-for-gc-things template that contains a js::HashMap), you can tweak the interface to your heart's desire.
(Assignee)

Comment 2

6 years ago
Created attachment 610761 [details] [diff] [review]
v1: Expose DefaultInitSize.

True.  I was thinking that since WeakMap derives from HashMap that I could insert myself as a shim between them and get a few of the trivial methods for free.  It turns out that this is ~3 lines of code, and we can't handle relookupOrAdd sanely anyway.  This was a dead end (as you probably predicted :-).  Thankfully, converting between the two schemes was surprisingly easy with find-and-replace.

I mostly suggested this change because I think that put(p, k) = v; is an incredibly hard-to-understand interface, but I'm fine with whatever you think is best here.

There is, however, one new thing that would be nice to have, now that I'm wrapping and not overriding: access to sDefaultInitSize.  The attached exposes this as DefaultInitSize.
Attachment #610296 - Attachment is obsolete: true
Attachment #610761 - Flags: review?(luke)

Comment 3

6 years ago
Comment on attachment 610761 [details] [diff] [review]
v1: Expose DefaultInitSize.

Review of attachment 610761 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/HashTable.h
@@ +984,5 @@
>  
>      Impl impl;
>  
>    public:
> +    const static unsigned DefaultInitSize = Impl::sDefaultInitSize;

sDefaultInitSize, please (also another one below)
Attachment #610761 - Flags: review?(luke) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 610940 [details] [diff] [review]
v2: Add back the s.

https://tbpl.mozilla.org/?tree=Try&rev=90226ed2a115
Attachment #610761 - Attachment is obsolete: true
Attachment #610940 - Flags: review+
(Assignee)

Comment 5

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a911e7d5cea1
https://hg.mozilla.org/mozilla-central/rev/a911e7d5cea1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 742438
You need to log in before you can comment on or make changes to this bug.