[webkit-reviews] review denied: [Bug 212115] Use OptionSet<DragDestinationAction> for mask values : [Attachment 399923] Patch v2

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 212115: Use OptionSet<DragDestinationAction> for mask values
https://bugs.webkit.org/show_bug.cgi?id=212115

Attachment 399923: Patch v2

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




--- Comment #7 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 399923
  --> https://bugs.webkit.org/attachment.cgi?id=399923
Patch v2

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

> Source/WTF/wtf/OptionSet.h:84
> +	   ASSERT_WITH_MESSAGE(!m_storage || hasOneBitSet(m_storage) ||
m_storage == std::numeric_limits<std::underlying_type_t<T>>::max(), "Enumerator
is not a zero, a positive power of two or has all bits set.");

I don't really like this.  If we did this we should probably have a
static_assert that std::underlying_type<T> is unsigned.

> Source/WebCore/page/DragActions.h:34
> +enum class DragDestinationAction : uint32_t {

This only needs to be 1 byte.  Can't we use a uint8_t?

> Source/WebCore/page/DragActions.h:35
> +    None  = 0,

Do we still need this?	The default constructor of OptionSet should take care
of this case.

> Source/WebCore/page/DragActions.h:39
> +    Any   = std::numeric_limits<uint32_t>::max()

What if instead of this we made a function that returned an
OptionSet<DragDestinationAction> called anyDragDestination()?