Closed Bug 740153 Opened 8 years ago Closed 8 years ago

Don't expose HashTable entries directly from HashMap/HashSet


(Core :: JavaScript Engine, defect)

Not set





(Reporter: terrence, Assigned: terrence)




(1 file, 2 obsolete files)

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)
Attachment #610296 - Flags: review?(luke) → review+
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.
Attached patch v1: Expose DefaultInitSize. (obsolete) — Splinter Review
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 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+
Closed: 8 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.