Bug 211396 – Make copying ListHashSet<Ref<>> and converting to a vector just work

• Status: ASSIGNED
• Component: Web Template Framework

[Daniel Bates]

ListHashSet<Ref<Node>> a;
ListHashSet<Ref<Node>> b = a;

Should just work.

Also copyToVector(a) should just work. But they don't because Ref<> deletes its copy ctr and copy assignment op and only exposes a copyRef() to perform a copy.

[Daniel Bates]

Created attachment 398408 [details]
Patch

[Daniel Bates]

Comment on attachment 398408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398408&action=review

I debated between "clone" and "copier" in terminology. I don't have a strong preference.

> Source/WTF/wtf/CloneValue.h:36
> +auto cloneValue(T&& v)

Could this be const T&? If not, could static assert that v is a const lvalue reference type. Maybe more than one static assert to to give human readable message if caller passes 1) non-reference 2) non const lvalue reference.

[Daniel Bates]

Comment on attachment 398408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398408&action=review

> Source/WTF/WTF.xcodeproj/project.pbxproj:698
> +		CEA7F303246085B50049FBC0 /* CloneValue.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CloneValue.h; sourceTree = "<group>"; };

Need to add to CMakeLists.txt.

[Daniel Bates]

Comment on attachment 398408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398408&action=review

> Source/WTF/wtf/CloneValue.h:28
> +namespace WTF {

Need to include  <type_traits>

>> Source/WTF/wtf/CloneValue.h:36
>> +auto cloneValue(T&& v)
> 
> Could this be const T&? If not, could static assert that v is a const lvalue reference type. Maybe more than one static assert to to give human readable message if caller passes 1) non-reference 2) non const lvalue reference.

v => value

[Daniel Bates]

Comment on attachment 398408 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=398408&action=review

> Source/WTF/wtf/RefPtr.h:249
> +template<typename T, typename U>
> +struct CloneValueHelper<RefPtr<T, U>> {
> +    static RefPtr<T, U> clone(const RefPtr<T, U>& value) { return value; }
> +};

Is this needed?

[Daniel Bates]

Created attachment 398415 [details]
Patch

[Daniel Bates]

Created attachment 398418 [details]
Patch

[Darin Adler]

Comment on attachment 398418 [details]
Patch

I see this as a problem with Ref, not with ListHashSet. I’d prefer that we not change the collections.

[Darin Adler]

Comment on attachment 398418 [details]
Patch

I’d prefer that we not change the collections to work around a design problem in the Ref class template itself.

[Daniel Bates]

(In reply to Darin Adler from comment #9)
> Comment on attachment 398418 [details]
> Patch
> 
> I’d prefer that we not change the collections to work around a design
> problem in the Ref class template itself.

What's the fix? Is it undelete the copy ctr and assignment? If so, that's easy! Also, if so, delete copyRef()?

[Darin Adler]

I think we need to decide whether we are OK reversing the original design decision of making Ref not copyable. If we are, then of course, it’s very easy to do, just need implement the copy constructor and normal assignment operators instead of deleting them. The copyRef operation becomes unnecessary, and removing it could be a follow-up step.

The original decision was motivated by a desire to cut down on invisible reference count churn. The same could be done with RefPtr. I don’t see a good reason to have the two classes be different in this respect.

I think the hard part is the deciding, not the code change.

[Alex Christensen]

Back to r? because Darin has unanswered questions.
I like this because most of the time an implicit copy of a Ref is unnecessary ref church (like why Functions are non-copyable), but when we are copying to a Vector we want to just copy.

[Darin Adler]

I’ve encountered trouble using Ref in more than just collections. For example, it was hard to write SimpleRange because I wanted it to be copyable. Multiple times I have to move from a Ref to a RefPtr, not because a null value is important, but because it’s easier to build data structures out of something that is copyable. One example I recall was difficulty using Ref in a Variant.

[Geoffrey Garen]

I agree that Ref and RefPtr should have the same policy for copying; the only difference between them should be nullability.

I don't have a strong opinion on whether copying should be explicit or implicit. It seems like a tradeoff: making copying easier makes accidental copying easier. So, both sides have a reasonable argument.

But maybe I do have an opinion. We've been adding RefPtr in a bunch of places, and yet, it has been a long long time since I've seen a performance regression caused by refcount churn. So, I'm inclined to support making copying easier.

[Alex Christensen]

Comment on attachment 398418 [details]
Patch

I guess we just decided to add a copy constructor to Ref instead of this.