| Summary: | Weak GC maps should be easier to use | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Geoffrey Garen <ggaren> | ||||||||
| Component: | New Bugs | Assignee: | Geoffrey Garen <ggaren> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | jer.noble, naginenis, ossy, ostap73, rniwa, webkit.review.bot | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Geoffrey Garen
2013-01-18 12:30:34 PST
Created attachment 183530 [details]
Patch
Comment on attachment 183530 [details]
Patch
Sam Weinig reviewed this.
Committed r140194: <http://trac.webkit.org/changeset/140194> Build fixes: Committed r140199: <http://trac.webkit.org/changeset/140199> Committed r140200: <http://trac.webkit.org/changeset/140200> Created attachment 183537 [details]
For some reason "using" doesn't work on linuxes. Replace it with "typedef".
Reopened because of build break. Comment on attachment 183537 [details]
For some reason "using" doesn't work on linuxes. Replace it with "typedef".
rs=me.
Committed r140211: <http://trac.webkit.org/changeset/140211> You could have avoided 2.5 hours and 20 commit long build breakage , if you waited more than 18 seconds for the EWS bots ... Reopening to attach new patch. Created attachment 183692 [details]
Patch
Committed r140284: <http://trac.webkit.org/changeset/140284> Comment on attachment 183692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=183692&action=review > Source/JavaScriptCore/runtime/WeakGCMap.h:73 > AddResult add(const KeyType& key, MappedPassInType value) > { > gcMapIfNeeded(); > - AddResult addResult = Base::add(key, value); > - if (!addResult.isNewEntry && !addResult.iterator->value) { // Found a zombie value. > + AddResult addResult = Base::add(key, nullptr); > + if (!addResult.iterator->value) { // New value or found a zombie value. > addResult.isNewEntry = true; > addResult.iterator->value = value; > } To me this function looks like HashMap::set. Three questions: 1) Why is it right to name the operation add rather than set? 2) Why doesn’t this call Base::set rather than Base::add? 3) Does HashMap::set have the same bug that you fixed here? If not, why not? > 3) Does HashMap::set have the same bug that you fixed here? If not, why not? I wouldn't call this a bug in HashMap -- it's just a unique feature of weak values. The key new feature in WeakGCMap, which isn't appropriate for HashSet, is that values can magically become null, and a null value is treated "as if" there were no such entry in the table. Perhaps HashMap could be extended to support this feature naturally through an additional value trait. Then we could remove WeakGCMap entirely. I'm not sure if that would be appropriate or not. > 1) Why is it right to name the operation add rather than set? If there is a non-null value in the table, this function will not overwrite the value. > 2) Why doesn’t this call Base::set rather than Base::add? Same reason. |