[webkit-reviews] review granted: [Bug 131685] std::bitset<>::test() does unnecessary bounds checks on CSSPropertyID bitsets : [Attachment 230197] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Zan Dobersek
<zandobersek at gmail.com>'s request for review:
Bug 131685: std::bitset<>::test() does unnecessary bounds checks on
CSSPropertyID bitsets
https://bugs.webkit.org/show_bug.cgi?id=131685

Attachment 230197: Patch
https://bugs.webkit.org/attachment.cgi?id=230197&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230197&action=review


> Source/WebCore/css/StyleProperties.cpp:928
> +	   ASSERT(!shorthandPropertyID || shortPropertyIndex <
shorthandPropertyUsed.size());

Wouldn’t it be better to put this assertion inside the if just below to avoid
the "||" here?

> Source/WebCore/css/StyleResolver.cpp:191
> -    bool hasProperty(CSSPropertyID id) const { return
m_propertyIsPresent.test(id); }
> +    bool hasProperty(CSSPropertyID id) const
> +    {
> +	   ASSERT(id < m_propertyIsPresent.size());
> +	   return m_propertyIsPresent[id];
> +    }

Normally when a function body grows to be nontrivial like this we like to put
it in the header after the class definition, with the inline keyword, rather
than leaving it inside the class definition.