[webkit-reviews] review granted: [Bug 211861] [IPC] Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes : [Attachment 399299] Patch v1

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 13 14:23:09 PDT 2020


Alex Christensen <achristensen at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 211861: [IPC] Use templates to reduce duplicate code in IPC::Decoder and
IPC::Encoder classes
https://bugs.webkit.org/show_bug.cgi?id=211861

Attachment 399299: Patch v1

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




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

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

> Source/WebKit/ChangeLog:3
> +	   [IPC] Use templates to reduce duplicate code in IPC::Decoder and
IPC::Encoder classes

This was intentionally not done earlier before because we wanted to avoid
serializing size_t because we supported 32-bit plugins in 64-bit hosts.  Now
that macOS doesn't support 32-bit executables, I think we can do this.

Do we want to do the same template code deduplication with PersistentEncoder
and PersistentDecoder?

> Source/WebKit/Platform/IPC/Decoder.h:127
> +	   // FIXME: Check that this is a valid enum value.

I believe decodeEnum is only used for decoding legacy enum types without
EnumTraits.  I think this FIXME should read: migrate all uses of this function
to operator>> with proper value checks and remove this.

> Source/WebKit/Platform/IPC/Encoder.h:67
> +	   // FIXME: Check that this is a valid enum value.

We typically don't do enum validity checking on the encoding side.  It's
unnecessary slowdown.  If it's a compromised process, they won't check anyways.

> Source/WebKit/Platform/IPC/Encoder.h:93
> +    template<typename T>
> +    auto encode(T value) -> std::enable_if_t<std::is_arithmetic<T>::value>

This syntax makes it very hard to see what is returned.  Could you use the same
syntax as everything else in this file and have two template parameters, the
second with std::enable_if_t?


More information about the webkit-reviews mailing list