Last Comment Bug 740153 - Don't expose HashTable entries directly from HashMap/HashSet
: Don't expose HashTable entries directly from HashMap/HashSet
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Terrence Cole [:terrence]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 742438
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-28 14:21 PDT by Terrence Cole [:terrence]
Modified: 2012-04-04 10:46 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0: and fix two bracket mis-alignments. (3.48 KB, patch)
2012-03-28 14:21 PDT, Terrence Cole [:terrence]
luke: review+
Details | Diff | Splinter Review
v1: Expose DefaultInitSize. (4.16 KB, patch)
2012-03-29 17:07 PDT, Terrence Cole [:terrence]
luke: review+
Details | Diff | Splinter Review
v2: Add back the s. (4.33 KB, patch)
2012-03-30 11:05 PDT, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-03-28 14:21:31 PDT
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.
Comment 1 Luke Wagner [:luke] 2012-03-28 14:26:28 PDT
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] 2012-03-29 17:07:31 PDT
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.
Comment 3 Luke Wagner [:luke] 2012-03-29 17:13:27 PDT
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] 2012-03-30 11:05:06 PDT
Created attachment 610940 [details] [diff] [review]
v2: Add back the s.

https://tbpl.mozilla.org/?tree=Try&rev=90226ed2a115
Comment 5 Terrence Cole [:terrence] 2012-04-02 10:38:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a911e7d5cea1
Comment 6 Marco Bonardo [::mak] 2012-04-03 01:59:42 PDT
https://hg.mozilla.org/mozilla-central/rev/a911e7d5cea1

Note You need to log in before you can comment on or make changes to this bug.