Bug 220410

Summary: WebKit::IPC::Encoder needs definitions of all enum types with custom validity checker at the Encoder definition time
Product: WebKit Reporter: Kimmo Kinnunen <kkinnunen>
Component: WebKit2Assignee: Kimmo Kinnunen <kkinnunen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 220319    
Bug Blocks: 217211    
Attachments:
Description Flags
Patch
none
Patch none

Kimmo Kinnunen
Reported 2021-01-07 05:36:43 PST
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.
Attachments
Patch (10.63 KB, patch)
2021-01-07 07:57 PST, Kimmo Kinnunen 		no flags
DetailsFormatted DiffDiff
Patch (10.08 KB, patch)
2021-01-07 23:11 PST, Kimmo Kinnunen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kimmo Kinnunen
Comment 1 2021-01-07 07:57:23 PST
Created attachment 417179 [details] Patch
Darin Adler
Comment 2 2021-01-07 11:10:01 PST
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
Comment 3 2021-01-07 23:11:48 PST
Created attachment 417246 [details] Patch
Kimmo Kinnunen
Comment 4 2021-01-07 23:22:14 PST
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
Comment 5 2021-01-11 05:04:31 PST
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
Comment 6 2021-01-11 05:05:25 PST
<rdar://problem/72994509>
Note You need to log in before you can comment on or make changes to this bug.