Bug 159837 - Add move constructor / assignment operator to ListHashSet
Summary: Add move constructor / assignment operator to ListHashSet
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-15 14:42 PDT by Chris Dumez
Modified: 2016-07-16 09:34 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.15 KB, patch)
2016-07-15 14:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2016-07-15 14:57 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (1.29 MB, application/zip)
2016-07-15 16:06 PDT, Build Bot
no flags Details
Patch (7.82 KB, patch)
2016-07-15 17:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-07-15 14:42:58 PDT
Add move constructor / assignment operator to ListHashSet.
Comment 1 Chris Dumez 2016-07-15 14:43:40 PDT
Created attachment 283793 [details]
Patch
Comment 2 Alex Christensen 2016-07-15 14:48:57 PDT
Comment on attachment 283793 [details]
Patch

As long as we're touching this file, let's pragma once it and make 0's nullptrs.
Comment 3 Chris Dumez 2016-07-15 14:57:01 PDT
Created attachment 283795 [details]
Patch
Comment 4 WebKit Commit Bot 2016-07-15 15:26:33 PDT
Comment on attachment 283795 [details]
Patch

Clearing flags on attachment: 283795

Committed r203304: <http://trac.webkit.org/changeset/203304>
Comment 5 WebKit Commit Bot 2016-07-15 15:26:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2016-07-15 15:35:58 PDT
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
Comment 7 Chris Dumez 2016-07-15 15:36:53 PDT
(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.
Comment 8 Chris Dumez 2016-07-15 15:39:23 PDT
Reverted r203304 for reason:

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

Committed r203306: <http://trac.webkit.org/changeset/203306>
Comment 9 Darin Adler 2016-07-15 15:39:37 PDT
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.
Comment 10 Chris Dumez 2016-07-15 15:41:43 PDT
(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.
Comment 11 Build Bot 2016-07-15 16:06:09 PDT
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
Comment 12 Build Bot 2016-07-15 16:06:14 PDT
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
Comment 13 Chris Dumez 2016-07-15 17:14:13 PDT
Created attachment 283824 [details]
Patch
Comment 14 WebKit Commit Bot 2016-07-16 09:34:02 PDT
Comment on attachment 283824 [details]
Patch

Clearing flags on attachment: 283824

Committed r203325: <http://trac.webkit.org/changeset/203325>
Comment 15 WebKit Commit Bot 2016-07-16 09:34:08 PDT
All reviewed patches have been landed.  Closing bug.