Bug 159837 – Add move constructor / assignment operator to ListHashSet

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Add move constructor / assignment operator to ListHashSet.

[Chris Dumez]

Created attachment 283793 [details]
Patch

[Alex Christensen]

Comment on attachment 283793 [details]
Patch

As long as we're touching this file, let's pragma once it and make 0's nullptrs.

[Chris Dumez]

Created attachment 283795 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283795 [details]
Patch

Clearing flags on attachment: 283795

Committed r203304: <http://trac.webkit.org/changeset/203304>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 283795 [details]
Patch

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

> Source/WTF/ChangeLog:3
> +        Add move constructor / assignment operator to ListHashSet

Why not add a unit test?

> Source/WTF/wtf/ListHashSet.h:73
> +    ListHashSet(ListHashSet&&) = default;

How can this be right? Doesn’t it leave the

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 283795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283795&action=review
> 
> > Source/WTF/ChangeLog:3
> > +        Add move constructor / assignment operator to ListHashSet
> 
> Why not add a unit test?
> 
> > Source/WTF/wtf/ListHashSet.h:73
> > +    ListHashSet(ListHashSet&&) = default;
> 
> How can this be right? Doesn’t it leave the

You did not finish your sentence.

[Chris Dumez]

Reverted r203304 for reason:

This is wrong because of Node* entries in the internal HashMap

Committed r203306: <http://trac.webkit.org/changeset/203306>

[Darin Adler]

Comment on attachment 283795 [details]
Patch

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

>> Source/WTF/wtf/ListHashSet.h:73
>> +    ListHashSet(ListHashSet&&) = default;
> 
> How can this be right? Doesn’t it leave the

Oops, I didn’t realize I included this. I had meant to delete it.

What I meant was: This leaves the list hash set on the right side in a state where you can’t use it. It’s common for us to instead reset the moved object to an empty state in our classes; for example, I think HashSet does this. This would require explicit code to set m_head and m_tail to nullptr. The downside is that this would be additional code that the compiler might not be smart enough to remove as a dead-store optimization. The upside is that this can be used in code that wants to move from an object and then count on it being valid empty object.

[Chris Dumez]

(In reply to comment #9)
> Comment on attachment 283795 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283795&action=review
> 
> >> Source/WTF/wtf/ListHashSet.h:73
> >> +    ListHashSet(ListHashSet&&) = default;
> > 
> > How can this be right? Doesn’t it leave the
> 
> Oops, I didn’t realize I included this. I had meant to delete it.
> 
> What I meant was: This leaves the list hash set on the right side in a state
> where you can’t use it. It’s common for us to instead reset the moved object
> to an empty state in our classes; for example, I think HashSet does this.
> This would require explicit code to set m_head and m_tail to nullptr. The
> downside is that this would be additional code that the compiler might not
> be smart enough to remove as a dead-store optimization. The upside is that
> this can be used in code that wants to move from an object and then count on
> it being valid empty object.

Ok, I'll look into adding a test and a better move constructor / assignment op.

[Build Bot]

Comment on attachment 283795 [details]
Patch

Attachment 283795 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1687903

New failing tests:
accessibility/mac/input-replacevalue-userinfo.html

[Build Bot]

Created attachment 283807 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Chris Dumez]

Created attachment 283824 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 283824 [details]
Patch

Clearing flags on attachment: 283824

Committed r203325: <http://trac.webkit.org/changeset/203325>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.