[webkit-reviews] review canceled: [Bug 220410] WebKit::IPC::Encoder needs definitions of all enum types with custom validity checker at the Encoder definition time : [Attachment 417246] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has canceled Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 220410: WebKit::IPC::Encoder needs definitions of all enum types with
custom validity checker at the Encoder definition time
https://bugs.webkit.org/show_bug.cgi?id=220410

Attachment 417246: Patch

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




--- Comment #4 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 417246
  --> https://bugs.webkit.org/attachment.cgi?id=417246
Patch

Thanks.

(In reply to Darin Adler from comment #2)
> Comment on attachment 417179 [details]
> > Source/WTF/wtf/EnumTraits.h:63
> > +auto isValidEnum(T t) -> decltype(EnumTraits<E>::isValidEnum(t), bool())
> 
> Why isn’t this one constexpr?

This would require the client-implementable function to be constexpr. None of
the current called functions can be usefully constexpr.

> > Source/WebCore/platform/ContextMenuItem.h:213
> > +	 static bool isValidEnum(T action)
> 
> Why isn’t this one constexpr?

There are no set of argument values such that an invocation of the function
could be an evaluated subexpression of a core constant expression.
Only expression is a call to non-inline (exported) function.

> > Source/WebKit/Scripts/webkit/messages.py:993
> > +	 result.append('    static bool isValidEnum(T messageName)\n')
> 
> Why isn’t this one constexpr?

Constexpr part is very narrow, and a constant function, false. It's not useful
as constexpr. The intended caller, e.g. the WTF::isValidEnum() is not constexpr
due to reasons above.

> 
> > Source/WebKit/Scripts/webkit/messages.py:996
> > +	 result.append('	static_assert(sizeof(T) ==
sizeof(IPC::MessageName), "isValidEnum<IPC::MessageName> should only be called
with 16-bit types");\n')
> > +	 result.append('	static_assert(std::is_unsigned<T>::value,
"isValidEnum<IPC::MessageName> should only be called with unsigned types");\n')
> 
> These could be done with enable_if instead of static_assert inside the
> function. How did you choose?

They were like this before, the patch did not touch them, only change the
indent IIRC.

I removed the static asserts as you suggested, here and in ContextMenuItem.