740153 - Don't expose HashTable entries directly from HashMap/HashSet

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 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.

Comment 3

[Luke Wagner [: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)

Comment 4

[Terrence Cole [:terrence]]

Attachment: v2: Add back the s.

https://tbpl.mozilla.org/?tree=Try&rev=90226ed2a115

Comment 5

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a911e7d5cea1

Comment 6

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/a911e7d5cea1