Bug 211396 - Make copying ListHashSet<Ref<>> and converting to a vector just work
Summary: Make copying ListHashSet<Ref<>> and converting to a vector just work
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-04 13:54 PDT by Daniel Bates
Modified: 2020-05-07 11:32 PDT (History)
20 users (show)

See Also:


Attachments
Patch (13.40 KB, patch)
2020-05-04 14:00 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2020-05-04 14:22 PDT, Daniel Bates
no flags Details | Formatted Diff | Diff
Patch (13.34 KB, patch)
2020-05-04 14:40 PDT, Daniel Bates
achristensen: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2020-05-04 13:54:00 PDT
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.
Comment 1 Daniel Bates 2020-05-04 14:00:16 PDT
Created attachment 398408 [details]
Patch
Comment 2 Daniel Bates 2020-05-04 14:03:55 PDT
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.
Comment 3 Daniel Bates 2020-05-04 14:06:20 PDT
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.
Comment 4 Daniel Bates 2020-05-04 14:12:50 PDT
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
Comment 5 Daniel Bates 2020-05-04 14:17:26 PDT
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?
Comment 6 Daniel Bates 2020-05-04 14:22:20 PDT
Created attachment 398415 [details]
Patch
Comment 7 Daniel Bates 2020-05-04 14:40:28 PDT
Created attachment 398418 [details]
Patch
Comment 8 Darin Adler 2020-05-04 15:51:51 PDT
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.
Comment 9 Darin Adler 2020-05-04 15:52:20 PDT
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.
Comment 10 Daniel Bates 2020-05-04 15:55:03 PDT
(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()?
Comment 11 Darin Adler 2020-05-04 15:59:01 PDT
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.
Comment 12 Alex Christensen 2020-05-04 22:01:31 PDT
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.
Comment 13 Darin Adler 2020-05-04 23:33:47 PDT
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.
Comment 14 Geoffrey Garen 2020-05-05 10:45:20 PDT
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.
Comment 15 Alex Christensen 2020-05-07 11:32:46 PDT
Comment on attachment 398418 [details]
Patch

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