[webkit-reviews] review granted: [Bug 215100] IndexSparseSet::sort() should update m_map : [Attachment 405862] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 215100: IndexSparseSet::sort() should update m_map
https://bugs.webkit.org/show_bug.cgi?id=215100

Attachment 405862: Patch

https://bugs.webkit.org/attachment.cgi?id=405862&action=review




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 405862
  --> https://bugs.webkit.org/attachment.cgi?id=405862
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405862&action=review

r=me.  Please also fix the case in IndexSparseSet::remove() where it is doing a
comparison of (m_values[position] == value) when it should be comparing
(EntryTypeTraits::key(m_values[position]) == value).

> Source/WTF/wtf/IndexSparseSet.h:103
> +    void validate();

Can you make this a no-op on !ASSERT_ENABLED builds i.e. an empty ALWAYS_INLINE
function on release builds?  I'm not sure it makes sense to pay the price of
this validation on a release build (and paying it twice too).

> Source/WTF/wtf/IndexSparseSet.h:240
> +template<typename EntryType, typename EntryTypeTraits, typename
OverflowHandler>
> +void IndexSparseSet<EntryType, EntryTypeTraits, OverflowHandler>::validate()
> +{
> +    for (unsigned val : *this)
> +	   RELEASE_ASSERT(contains(val));
>  }

Make this conditional on #if ASSERT_ENABLED.

While you're at it, can you also ASSERT that m_values.size() <= m_map.size()?