Make HashMap and HashSet work with Refs
Created attachment 281059 [details] Patch
First pass, may need some tweaking.
Attachment 281059 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Ref.h:118: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/Ref.h:122: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WTF/wtf/HashFunctions.h:201: More than one command on the same line [whitespace/newline] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
HashMap.take can lead to null Ref's, like this: HashMap<int, Ref<RefLogger>> map; RefLogger a("a"); Ref<RefLogger> ref(a); map.add(1, WTFMove(ref)); //Ref<RefLogger> v = map.get(1); // get returns a RefLogger*, so this doesn't compile. Great! RefPtr<RefLogger> w = map.get(1); // This does exactly what's expected. Great! Ref<RefLogger> x = map.take(1); // This does what's expected. Ref<RefLogger> y = map.take(1); // This makes a null Ref :( Ref<RefLogger> z = map.take(2); // This makes a null Ref :( WTFLogAlways("A %p W %p X %p Y %p Z %p", &a, *(void**)(&w), *(void**)(&x), *(void**)(&y), *(void**)(&z));
Not as big a disaster as you might think. After all, there are even simpler ways to make a null Ref: RefLogger a("a"); Ref<RefLogger> b(a); Ref<RefLogger> c = WTFMove(ref); // This makes b into a null Ref. RefLogger& d = c.leakRef(); // This makes c into a null Ref. I think it’s true, however, that we would want the take function to return Optional<Ref> or RefPtr instead of returning a null Ref. Probably worthwhile to return here and make that refinement.
Comment on attachment 281059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281059&action=review > Source/WTF/wtf/GetPtr.h:29 > +enum HashTableDeletedValueType { HashTableDeletedValue }; > +enum HashTableEmptyValueType { HashTableEmptyValue }; Sneaky place to put this; no real logical reason for it to be here. I guess it’s OK, treating this as the “common useful pointer-y stuff” header. > Source/WTF/wtf/HashTraits.h:176 > + static Ref<P> emptyValue() { return Ref<P>(HashTableEmptyValue); } Since the constructor is not marked explicit, I don’t think you need to use Ref<P>() around HashTableEmptyValue here.
(In reply to comment #5) > Not as big a disaster as you might think. After all, there are even simpler > ways to make a null Ref: > > RefLogger a("a"); > Ref<RefLogger> b(a); > Ref<RefLogger> c = WTFMove(ref); // This makes b into a null Ref. > RefLogger& d = c.leakRef(); // This makes c into a null Ref. > > I think it’s true, however, that we would want the take function to return > Optional<Ref> or RefPtr instead of returning a null Ref. Probably worthwhile > to return here and make that refinement. I was considering having take() return a RefPtr<>, but Optional<Ref<>> would work too. Do you have a preference?
(In reply to comment #7) > I was considering having take() return a RefPtr<>, but Optional<Ref<>> would > work too. Do you have a preference? I would prefer RefPtr.
(In reply to comment #8) > (In reply to comment #7) > > I was considering having take() return a RefPtr<>, but Optional<Ref<>> would > > work too. Do you have a preference? > I would prefer RefPtr. If we do Optional, it should return Optional for all types.
Comment on attachment 281059 [details] Patch Clearing flags on attachment: 281059 Committed r202002: <http://trac.webkit.org/changeset/202002>
All reviewed patches have been landed. Closing bug.
Reverted r202002 and r202111 for reason: Ryosuke says this was a JSBench regression on iOS Committed r202136: <http://trac.webkit.org/changeset/202136>
r202254 reverted r202136.
Comment on attachment 281059 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=281059&action=review > Source/WTF/wtf/HashTraits.h:185 > + } This code has a problem. Filed: Bug 215550 – [WTF] HashTable<Ref<K>, V>::HashTable(const HashTable& other) can't compile