Issue 6279 - Make HashContainer movable and optimize resizing by moving entries.

Issue 6279 - Make HashContainer movable and optimize resizing by moving entries.

[sergei, 2018-01-24 18:37:12]

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/Map.h
File compiled/Map.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/Map.h#newc...
compiled/Map.h:165: entry_type* _assign(entry_type* existing, const entry_type&
entry)
We normally don't use underscores at the beginning of the names, what about
something like `incrementSize` (something better is very welcomed)? IMO it
should reflect that the function increases mEntryCount if necessary and adjusts
the capacity in accordance with the LOAD_FACTOR.

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap....
compiled/StringMap.h:57: StringSetEntry& operator=(StringSetEntry&&) = default;
It seems it's actually not required here and in StringMapEntry, because at least
all requirements for implicitly generated constructor and assignment operator
are satisfied.
http://en.cppreference.com/w/cpp/language/copy_assignment and others.

[hub, 2018-01-24 19:32:01]

updated

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/Map.h
File compiled/Map.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/Map.h#newc...
compiled/Map.h:165: entry_type* _assign(entry_type* existing, const entry_type&
entry)
On 2018/01/24 18:37:12, sergei wrote:
> We normally don't use underscores at the beginning of the names, what about
> something like `incrementSize` (something better is very welcomed)? IMO it
> should reflect that the function increases mEntryCount if necessary and
adjusts
> the capacity in accordance with the LOAD_FACTOR.

I'll call it "prepare_bucket" as we use it to ensure we have a bucket to assign
the entry to. Also I'll change the argument to just take the key instead of the
actual entry.

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap....
compiled/StringMap.h:57: StringSetEntry& operator=(StringSetEntry&&) = default;
On 2018/01/24 18:37:12, sergei wrote:
> It seems it's actually not required here and in StringMapEntry, because at
least
> all requirements for implicitly generated constructor and assignment operator
> are satisfied.
> http://en.cppreference.com/w/cpp/language/copy_assignment and others.

ah right.

Done

[sergei, 2018-01-25 10:55:07]

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap....
compiled/StringMap.h:57: StringSetEntry& operator=(StringSetEntry&&) = default;
On 2018/01/24 19:32:01, hub wrote:
> On 2018/01/24 18:37:12, sergei wrote:
> > It seems it's actually not required here and in StringMapEntry, because at
> least
> > all requirements for implicitly generated constructor and assignment
operator
> > are satisfied.
> > http://en.cppreference.com/w/cpp/language/copy_assignment and others.
> 
> ah right.
> 
> Done

Could you please also do it for StringMapEntry below?

[sergei, 2018-01-25 11:06:26]

I think the commit message should be also changed, it's essentially making
HashContainer based classes movable and a couple of optimizations, which are
moving of its entries internally and moving of a value parameter in the entry
construcror.

[hub, 2018-01-25 15:11:11]

updated. changed commit message (and review title)

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap.h
File compiled/StringMap.h (right):

https://codereview.adblockplus.org/29677755/diff/29677823/compiled/StringMap....
compiled/StringMap.h:57: StringSetEntry& operator=(StringSetEntry&&) = default;
On 2018/01/25 10:55:07, sergei wrote:
> On 2018/01/24 19:32:01, hub wrote:
> > On 2018/01/24 18:37:12, sergei wrote:
> > > It seems it's actually not required here and in StringMapEntry, because at
> > least
> > > all requirements for implicitly generated constructor and assignment
> operator
> > > are satisfied.
> > > http://en.cppreference.com/w/cpp/language/copy_assignment and others.
> > 
> > ah right.
> > 
> > Done
> 
> Could you please also do it for StringMapEntry below?

Done.

(I was convinced they were needed, must have been a side effect, done now)

[sergei, 2018-01-25 15:20:57]

On 2018/01/25 15:11:11, hub wrote:
...
> (I was convinced they were needed, must have been a side effect, done now)

Maybe they are still needed but theoretically should not be :) let's see whether
and how the rest code is compiled.

LGTM