Deque: Add HeapDeque and prevent buggy use of swap and operator=

Deque: Add HeapDeque and prevent buggy use of swap and operator=

R=ager@chromium.org, jochen@chromium.org, mikhail.pozdynakov@intel.com
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171121

[Mads Ager (chromium), 2014-04-08 10:46:17]

There seems to be a file missing. :)

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1773: COMPILE_ASSERT(!Traits::isWeak,
WeDontSupportWeaknessInHeapVectors); // We don't support it in HeapDeques
either.
Put the comment on its own line above the COMPILE_ASSERT?

https://codereview.chromium.org/228403002/diff/1/Source/wtf/wtf.gypi
File Source/wtf/wtf.gypi (right):

https://codereview.chromium.org/228403002/diff/1/Source/wtf/wtf.gypi#newcode208
Source/wtf/wtf.gypi:208: 'DequeTest.cpp',
You need to add this file to the code review.

[wibling-chromium, 2014-04-08 10:49:40]

lgtm except for the missing file:)

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1438: Deque<T, inlineCapacity,
HeapAllocator>::swap(other);
NIT: Consider adding a comment about this only working for inlineCapacity == 0.

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1773: COMPILE_ASSERT(!Traits::isWeak,
WeDontSupportWeaknessInHeapVectors); // We don't support it in HeapDeques
either.
WeDontSupportWeaknessInHeapVectorsOrDeques?

[Mikhail, 2014-04-08 11:03:01]

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h
File Source/wtf/Deque.h (right):

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode49
Source/wtf/Deque.h:49: class DequeDestructorBase {
This looks similar to the existing VectorDestructorBase, can we re-use it?

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode117
Source/wtf/Deque.h:117: // assignment operator above.
did not get how we wire the inlineCapacity to zero for 'swap' (unlike assignment
operator), could you pls give more details?

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode318
Source/wtf/Deque.h:318: if (LIKELY(!m_buffer.buffer()))
so not having the buffer is most expected?

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode570
Source/wtf/Deque.h:570: for (const T* bufferEntry = bufferBegin + m_start;
bufferEntry != end; bufferEntry++)
nit: ++bufferEntry

[Erik Corry, 2014-04-08 11:48:52]

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1438: Deque<T, inlineCapacity,
HeapAllocator>::swap(other);
On 2014/04/08 10:49:40, wibling-chromium wrote:
> NIT: Consider adding a comment about this only working for inlineCapacity ==
0.

Done.

https://codereview.chromium.org/228403002/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1773: COMPILE_ASSERT(!Traits::isWeak,
WeDontSupportWeaknessInHeapVectors); // We don't support it in HeapDeques
either.
On 2014/04/08 10:49:40, wibling-chromium wrote:
> WeDontSupportWeaknessInHeapVectorsOrDeques?

Done.

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h
File Source/wtf/Deque.h (right):

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode49
Source/wtf/Deque.h:49: class DequeDestructorBase {
On 2014/04/08 11:03:01, mikhail.pozdnyakov wrote:
> This looks similar to the existing VectorDestructorBase, can we re-use it?

Good point, yes we can!

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode117
Source/wtf/Deque.h:117: // assignment operator above.
On 2014/04/08 11:03:01, mikhail.pozdnyakov wrote:
> did not get how we wire the inlineCapacity to zero for 'swap' (unlike
assignment
> operator), could you pls give more details? 

It seems I didn't wire the inlineCapacity to zero after all.  Fixed.  Also
replaced both comments with a reference to the bug.

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode318
Source/wtf/Deque.h:318: if (LIKELY(!m_buffer.buffer()))
On 2014/04/08 11:03:01, mikhail.pozdnyakov wrote:
> so not having the buffer is most expected?

Removed the LIKELY, as I don't think it really adds anything.  Also a few
cosmetic changes.

https://codereview.chromium.org/228403002/diff/1/Source/wtf/Deque.h#newcode570
Source/wtf/Deque.h:570: for (const T* bufferEntry = bufferBegin + m_start;
bufferEntry != end; bufferEntry++)
On 2014/04/08 11:03:01, mikhail.pozdnyakov wrote:
> nit: ++bufferEntry

Style guide allows both.

https://codereview.chromium.org/228403002/diff/1/Source/wtf/wtf.gypi
File Source/wtf/wtf.gypi (right):

https://codereview.chromium.org/228403002/diff/1/Source/wtf/wtf.gypi#newcode208
Source/wtf/wtf.gypi:208: 'DequeTest.cpp',
On 2014/04/08 10:46:17, Mads Ager (chromium) wrote:
> You need to add this file to the code review.

Done.

[Mikhail, 2014-04-08 12:11:29]

lgtm

https://codereview.chromium.org/228403002/diff/60001/Source/wtf/Deque.h
File Source/wtf/Deque.h (right):

https://codereview.chromium.org/228403002/diff/60001/Source/wtf/Deque.h#newco...
Source/wtf/Deque.h:56: {
Off topic: the same allocation operators are copy-pasted in several container
classes already. Looks like it deserves having a macro, similar to
'WTF_MAKE_FAST_ALLOCATED'

https://codereview.chromium.org/228403002/diff/60001/Source/wtf/Deque.h#newco...
Source/wtf/Deque.h:63: {
is this compatible with GC? shouldn't
COMPILE_ASSERT(!Allocator::isGarbageCollected) be added?

[Mads Ager (chromium), 2014-04-08 12:13:13]

LGTM

[Erik Corry, 2014-04-08 12:14:35]

https://codereview.chromium.org/228403002/diff/60001/Source/wtf/Deque.h
File Source/wtf/Deque.h (right):

https://codereview.chromium.org/228403002/diff/60001/Source/wtf/Deque.h#newco...
Source/wtf/Deque.h:63: {
On 2014/04/08 12:11:30, mikhail.pozdnyakov wrote:
> is this compatible with GC? shouldn't
> COMPILE_ASSERT(!Allocator::isGarbageCollected) be added?

We do allow heap collections to be off heap, but the backing is always on heap. 
So heap collections can be on the stack or inside other objects, either other
heap collections or other objects.  Therefore it is possible to placement new
them. If you do that and then put a pointer to them in a Member you are busted.

[zerny-chromium, 2014-04-08 12:38:50]

lgtm

https://codereview.chromium.org/228403002/diff/60001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/228403002/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:1523: // a specialization of GCInfoTrait for these
three classes here.
Nit: three => four, or remove the count.

[jochen (gone - plz use gerrit), 2014-04-08 13:32:51]

lgtm

[Erik Corry, 2014-04-08 13:50:03]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-04-08 13:50:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/228403002/100001

[commit-bot: I haz the power, 2014-04-08 14:08:10]

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

[commit-bot: I haz the power, 2014-04-08 14:08:10]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[Erik Corry, 2014-04-09 07:09:56]

The CQ bit was checked by erik.corry@gmail.com

[commit-bot: I haz the power, 2014-04-09 07:10:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erik.corry@gmail.com/228403002/100001

[commit-bot: I haz the power, 2014-04-09 07:36:07]

Change committed as 171121