WTF::HashTable: Implement move semantics for keys and values.

WTF::HashTable: Implement move semantics for keys and values.

This patch essentially updates all value-accepting member functions so
they could accept universal references instead of const lvalue
references.

Some use sites need to be updated so the types of the arguments are
correctly inferred.

This patch only touches HashMap, and the other HashTable-based
containers will be done later. They can be fixed in the same way as
HashMap.

BUG=567139, 582349
Committed: https://crrev.com/893a6b1187d3316943fcf19cee65aa5ab22fef7f
Cr-Commit-Position: refs/heads/master@{#379527}

[Yuta Kitamura, 2016-03-04 11:07:01]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-04 11:07:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1764973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1764973002/1

[Yuta Kitamura, 2016-03-04 12:26:36]

yutak@chromium.org changed reviewers:
+ haraken@chromium.org, mikhail.pozdnyakov@intel.com, tzik@chromium.org

[Yuta Kitamura, 2016-03-04 12:26:37]

PTAL

[commit-bot: I haz the power, 2016-03-04 12:30:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-04 12:30:09]

Dry run: This issue passed the CQ dry run.

[Mikhail, 2016-03-04 12:58:02]

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashMap.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashMap.h:376: HashMap<T, U, V, W, X,
Y>::inlineAdd(IncomingKeyType&& key, IncomingMappedType&& mapped)
think it's worth switching to 'auto HashMap<T, U, V, W, X, Y>::inlineAdd() ->
AddResult' syntax

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTable.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTable.h:486: template <typename
HashTranslator, typename T> ValueType* lookup(const T&);
why not T&& ?

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTraits.h:115: static T&& passOut(T& value) {
return std::move(value); }
do we still need 'passOut'? can std::move be called directly?

[Yuta Kitamura, 2016-03-04 13:55:37]

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashMap.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashMap.h:376: HashMap<T, U, V, W, X,
Y>::inlineAdd(IncomingKeyType&& key, IncomingMappedType&& mapped)
On 2016/03/04 12:58:02, Mikhail wrote:
> think it's worth switching to 'auto HashMap<T, U, V, W, X, Y>::inlineAdd() ->
> AddResult' syntax

That syntax is "to be discussed" section in chromium-cpp.appspot.com,
so I think it's premature to use it now.

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTable.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTable.h:486: template <typename
HashTranslator, typename T> ValueType* lookup(const T&);
On 2016/03/04 12:58:02, Mikhail wrote:
> why not T&& ?

This is used to calculate the hash value of T, so this function does
not employ ownership movement. Thus we don't have to (and actually
can't) use T&& here.

The reason of changing |T| to |const T&| is to make it possible to
calculate a hash value of non-copyable (move-only) T. The old code
did a copy here.

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTraits.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTraits.h:115: static T&& passOut(T& value) {
return std::move(value); }
On 2016/03/04 12:58:02, Mikhail wrote:
> do we still need 'passOut'? can std::move be called directly?

Other specializations (for e.g. PassRefPtr) still require it.

Eventually we could get rid of this, though, after PassOwnPtr
and PassRefPtr acquire proper move semantics instead of copying.

[Mikhail, 2016-03-04 14:19:26]

On 2016/03/04 13:55:37, Yuta Kitamura wrote:
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/HashMap.h (right):
> 
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/HashMap.h:376: HashMap<T, U, V, W, X,
> Y>::inlineAdd(IncomingKeyType&& key, IncomingMappedType&& mapped)
> On 2016/03/04 12:58:02, Mikhail wrote:
> > think it's worth switching to 'auto HashMap<T, U, V, W, X, Y>::inlineAdd()
->
> > AddResult' syntax
> 
> That syntax is "to be discussed" section in http://chromium-cpp.appspot.com,
> so I think it's premature to use it now.

Think you can use it https://codereview.chromium.org/1176303008/#msg6
> 
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/HashTable.h (right):
> 
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/HashTable.h:486: template <typename
> HashTranslator, typename T> ValueType* lookup(const T&);
> On 2016/03/04 12:58:02, Mikhail wrote:
> > why not T&& ?
> 
> This is used to calculate the hash value of T, so this function does
> not employ ownership movement. Thus we don't have to (and actually
> can't) use T&& here.
> 
> The reason of changing |T| to |const T&| is to make it possible to
> calculate a hash value of non-copyable (move-only) T. The old code
> did a copy here.
> 
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/HashTraits.h (right):
> 
>
https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/HashTraits.h:115: static T&& passOut(T& value) {
> return std::move(value); }
> On 2016/03/04 12:58:02, Mikhail wrote:
> > do we still need 'passOut'? can std::move be called directly?
> 
> Other specializations (for e.g. PassRefPtr) still require it.
> 
> Eventually we could get rid of this, though, after PassOwnPtr
> and PassRefPtr acquire proper move semantics instead of copying.

Both OwnPtr and RefPtr support move semantic atm, I believe we can get rid of
both 'passOut' and 'PassOutType' in traits. But that can be made in a separate
patch if you prefer.

[tzik, 2016-03-04 15:29:59]

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTable.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTable.h:810: unsigned h =
HashTranslator::hash(key);
This is not directly related to this CL, but it makes me uneasy to use |key|
multiple times in this function.

If T is a different type to Key and is implicitly convertible to Key, e.g. T =
const char*, and Key = WTF::String, then each conversion from T&& to const Key&
will run Key::Key(const T&).
So, I think each HashTranslator::hash(key) and HashTranslator::equal(..., key)
make one WTF::String if the user passed a "foo" as |key|.

[tzik, 2016-03-07 03:50:10]

lgtm

[haraken, 2016-03-07 04:12:14]

LGTM

[Yuta Kitamura, 2016-03-07 04:24:37]

Mikhail: From the next time, please reply from the inline comments
so your replies would be properly attached to them.

On 2016/03/04 14:19:26, Mikhail wrote:
> Think you can use it https://codereview.chromium.org/1176303008/#msg6

Hmm, I still think having a consensus in cxx@chromium.org is good,
and I'm afraid that changing only this single instance to trailing
return type style would make the code inconsistent and less readable.

Since this isn't something newly introduced in this patch, I would
like to leave this as is. IMO, the style should be changed at once,
preferably after we get the consensus about the syntax.

> 
> Both OwnPtr and RefPtr support move semantic atm, I believe we can get rid of
> both 'passOut' and 'PassOutType' in traits. But that can be made in a separate
> patch if you prefer.

Yeah I think you are right but I'm a little hesitant to do it because
they are kind of part of HashTable API, along with peekIn, passIn and
peekOut. Removing just one out of these sounds a little bit awkward.

[Yuta Kitamura, 2016-03-07 04:36:27]

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/HashTable.h (right):

https://codereview.chromium.org/1764973002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/HashTable.h:810: unsigned h =
HashTranslator::hash(key);
On 2016/03/04 15:29:59, tzik wrote:
> This is not directly related to this CL, but it makes me uneasy to use |key|
> multiple times in this function.
> 
> If T is a different type to Key and is implicitly convertible to Key, e.g. T =
> const char*, and Key = WTF::String, then each conversion from T&& to const
Key&
> will run Key::Key(const T&).
> So, I think each HashTranslator::hash(key) and HashTranslator::equal(..., key)
> make one WTF::String if the user passed a "foo" as |key|.

I don't plan to do that in this patch, but sounds plausible.

I think the complication here is we don't have a permanent storage for
key at this point, because that is what we are trying to find in this
function. Hmmm...

[Mikhail, 2016-03-07 07:11:23]

lgtm

[Yuta Kitamura, 2016-03-07 07:11:57]

Thanks!

[Yuta Kitamura, 2016-03-07 07:12:06]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2016-03-07 07:12:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1764973002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1764973002/1

[commit-bot: I haz the power, 2016-03-07 08:34:46]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-07 08:35:40]

Description was changed from

==========
WTF::HashTable: Implement move semantics for keys and values.

This patch essentially updates all value-accepting member functions so
they could accept universal references instead of const lvalue
references.

Some use sites need to be updated so the types of the arguments are
correctly inferred.

This patch only touches HashMap, and the other HashTable-based
containers will be done later. They can be fixed in the same way as
HashMap.

BUG=567139,582349
==========

to

==========
WTF::HashTable: Implement move semantics for keys and values.

This patch essentially updates all value-accepting member functions so
they could accept universal references instead of const lvalue
references.

Some use sites need to be updated so the types of the arguments are
correctly inferred.

This patch only touches HashMap, and the other HashTable-based
containers will be done later. They can be fixed in the same way as
HashMap.

BUG=567139,582349

Committed: https://crrev.com/893a6b1187d3316943fcf19cee65aa5ab22fef7f
Cr-Commit-Position: refs/heads/master@{#379527}
==========

[commit-bot: I haz the power, 2016-03-07 08:35:41]

Patchset 1 (id:??) landed as
https://crrev.com/893a6b1187d3316943fcf19cee65aa5ab22fef7f
Cr-Commit-Position: refs/heads/master@{#379527}