WebKit::IPC::Encoder needs definitions of all enum types with custom validity checker at the Encoder definition time

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

WebKit::IPC::Encoder needs definitions of all enum values at the Encoder definition time

This is a problem because Encoder is used to encode arbitrary content. 

The expectation is that all the definitions would be needed at _instantiation time_. This is true for other encoded types, but not for enums.

The source is that WTF::isValidEnum() is defined as few functions.
It is the intention of the original writer that clients "override" these isValidEnum() functions for the custom enums, and then the encoder would use these.

However, since the functions are freestanding functions, they do not share a base function template that would be then overridden by specializations of the function templates.
C++ does not have specialised function templates.

This means that calls made by Encoder into WTF::isValidEnum() are matched at definition time. If the custom enums are included, then the compile works. If the custom enums are not included at this point, the compile fails.

[Kimmo Kinnunen]

Created attachment 417179 [details]
Patch

[Darin Adler]

Comment on attachment 417179 [details]
Patch

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

> Source/WTF/wtf/EnumTraits.h:63
> +auto isValidEnum(T t) -> decltype(EnumTraits<E>::isValidEnum(t), bool())

Why isn’t this one constexpr?

> Source/WebCore/platform/ContextMenuItem.h:213
> +    static bool isValidEnum(T action)

Why isn’t this one constexpr?

> Source/WebKit/Scripts/webkit/messages.py:993
> +    result.append('    static bool isValidEnum(T messageName)\n')

Why isn’t this one constexpr?

> 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?

[Kimmo Kinnunen]

Created attachment 417246 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 417246 [details]
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.

[EWS]

Committed r271358: <https://trac.webkit.org/changeset/271358>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417246 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72994509>