Introduce TerminatedArray<T> type to make data-structure used by RuleSet visible at the type level.

Introduce TerminatedArray<T> type to make data-structure used by RuleSet visible at the type level.

Before this change it was not possible to easily distinguish when RuleData represented a single item and when it represented an array.

This change is prerequisite for moving RuleSet/RuleData into the GC managed heap because tracing routines are largely type driven.

BUG=
NOTRY=true

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

[Vyacheslav Egorov (Chromium), 2014-03-04 17:18:02]

Adam, could you take a look?

According to commit logs it was you who introduced terminated arrays of
RuleData.

Thanks.

[abarth-chromium, 2014-03-04 19:03:35]

LGTM

Very nice

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:118: class TerminatedArray {
Can you move this into it's own header in WTF?

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:131: T& at(size_t idx) { return
reinterpret_cast<T*>(this)[idx]; }
idx -> index

[haraken, 2014-03-05 01:47:05]

LGTM

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:119: WTF_MAKE_FAST_ALLOCATED;

Add WTF_MAKE_NONCOPYABLE(TerminatedArray).

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:126: PassOwnPtr<TerminatedArray> realloc(size_t
capacity)

realloc => resize ?

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:132: const T& at(size_t idx) const { return
reinterpret_cast<const T*>(this)[idx]; }

Shall we add operator[] ?

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:149: bool operator!=(const iterator_base& other) {
return m_val != other.m_val; }

Shall we add operator== ?

[wibling-chromium, 2014-03-05 09:28:32]

lgtm

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:168: size_t size() const
Consider adding a comment describing how this datastructure should be used.
AFAICT you cannot iterate over the elements to determine the size unless the
caller has ensured there are no holes (empty elements) inline with non-empty
elements. Also it might be worth adding that only types with an isLastInArray
method can be used.

[Erik Corry, 2014-03-05 11:43:51]

lgtm

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:118: class TerminatedArray {
On 2014/03/04 19:03:35, abarth wrote:
> Can you move this into it's own header in WTF?

I think this is rather too hacky to move to WTF.  There are a lot of potential
footguns in this code and it's only used one place.  I think once we are fully
moved to Oilpan we can make Vector work well enough that this is no longer
necessary and we can get rid of it again.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:123: return
adoptPtr(static_cast<TerminatedArray*>(fastMalloc(capacity * sizeof(T))));
Consider using zeroed malloc here, since normally the constructor for T is not
called.

[Vyacheslav Egorov (Chromium), 2014-03-05 12:44:07]

Comments addressed.

Can somebody take another quick look?

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h
File Source/core/css/RuleSet.h (right):

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:118: class TerminatedArray {
On 2014/03/05 11:43:52, Erik Corry wrote:
> On 2014/03/04 19:03:35, abarth wrote:
> > Can you move this into it's own header in WTF?
> 
> I think this is rather too hacky to move to WTF.  There are a lot of potential
> footguns in this code and it's only used one place.  I think once we are fully
> moved to Oilpan we can make Vector work well enough that this is no longer
> necessary and we can get rid of it again.

I decided to move both TerminatedArray and TerminatedArrayBuilder into separate
files and into WTF namespace. 

I agree with Erik that they are quite hackish and preferably should go away, but
right now having them in the WTF seems like the right thing to do especially in
the light of forthcomming changes that are going to parameterize TerminatedArray
with HeapAllocator. 

Having all these details in the RuleSet.h seems like incorrect thing to do.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:119: WTF_MAKE_FAST_ALLOCATED;
On 2014/03/05 01:47:06, haraken wrote:
> 
> Add WTF_MAKE_NONCOPYABLE(TerminatedArray).

Done.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:123: return
adoptPtr(static_cast<TerminatedArray*>(fastMalloc(capacity * sizeof(T))));
On 2014/03/05 11:43:52, Erik Corry wrote:
> Consider using zeroed malloc here, since normally the constructor for T is not
> called.

I will all be filled with real objects, so zeroing memory is not necessary. I
will move create() and realloc() to private section and make
TerminatedArrayBuilder a friend of this class.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:126: PassOwnPtr<TerminatedArray> realloc(size_t
capacity)
On 2014/03/05 01:47:06, haraken wrote:
> 
> realloc => resize ?

Done.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:131: T& at(size_t idx) { return
reinterpret_cast<T*>(this)[idx]; }
On 2014/03/04 19:03:35, abarth wrote:
> idx -> index

Done.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:132: const T& at(size_t idx) const { return
reinterpret_cast<const T*>(this)[idx]; }
On 2014/03/05 01:47:06, haraken wrote:
> 
> Shall we add operator[] ?

It will be useless because we pass these arrays by pointer everywhere right now.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:149: bool operator!=(const iterator_base& other) {
return m_val != other.m_val; }
On 2014/03/05 01:47:06, haraken wrote:
> 
> Shall we add operator== ?

Done.

https://codereview.chromium.org/181403005/diff/1/Source/core/css/RuleSet.h#ne...
Source/core/css/RuleSet.h:168: size_t size() const
On 2014/03/05 09:28:32, wibling-chromium wrote:
> Consider adding a comment describing how this datastructure should be used.
> AFAICT you cannot iterate over the elements to determine the size unless the
> caller has ensured there are no holes (empty elements) inline with non-empty
> elements. Also it might be worth adding that only types with an isLastInArray
> method can be used.

Done.

[wibling-chromium, 2014-03-05 12:54:35]

lgtm

[Vyacheslav Egorov (Chromium), 2014-03-05 13:03:14]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-03-05 13:03:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/181403005/20001

[haraken, 2014-03-05 13:58:46]

LGTM

[Vyacheslav Egorov (Chromium), 2014-03-05 17:40:56]

The CQ bit was unchecked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 17:40:57]

The CQ bit was checked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 17:48:34]

The CQ bit was unchecked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 17:48:34]

The CQ bit was checked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 17:48:35]

The CQ bit was unchecked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 17:48:40]

The CQ bit was checked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 18:11:41]

The CQ bit was unchecked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 18:11:47]

The CQ bit was checked by vegorov@chromium.org

[Vyacheslav Egorov (Chromium), 2014-03-05 18:26:11]

The CQ bit was checked by vegorov@chromium.org

[commit-bot: I haz the power, 2014-03-05 18:26:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vegorov@chromium.org/181403005/60001

[commit-bot: I haz the power, 2014-03-05 18:27:23]

Change committed as 168508