Bug 158638 – Make HashMap and HashSet work with Refs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Make HashMap and HashSet work with Refs

[Sam Weinig]

Created attachment 281059 [details]
Patch

[Sam Weinig]

First pass, may need some tweaking.

[WebKit Commit Bot]

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.

[Alex Christensen]

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));

[Darin Adler]

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.

[Darin Adler]

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.

[Sam Weinig]

(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?

[Alex Christensen]

(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.

[Alex Christensen]

(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.

[WebKit Commit Bot]

Comment on attachment 281059 [details]
Patch

Clearing flags on attachment: 281059

Committed r202002: <http://trac.webkit.org/changeset/202002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Reverted r202002 and r202111 for reason:

Ryosuke says this was a JSBench regression on iOS

Committed r202136: <http://trac.webkit.org/changeset/202136>

[Fujii Hironori]

r202254 reverted r202136.

[Fujii Hironori]

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