Use templates to reduce duplicate code in IPC::Decoder and IPC::Encoder classes. <rdar://problem/62360390>
Created attachment 399299 [details] Patch v1
Comment on attachment 399299 [details] 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?
Comment on attachment 399299 [details] 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? Good to know the history here. Yes, we could do this to PersistentEncoder and PersistentDecoder in a separate patch. >> 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. Sure, I can change this. I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it. >> 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. It's more about catching our own mistakes. This method has a Debug assertion: template<typename E> auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value> { ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value))); encode(static_cast<typename std::underlying_type<E>::type>(value)); } I assume that's okay to add? So change "Check" to "Add Debug assert"? Do we eventually want to get rid of encodeEnum() (just like decodeEnum()) and just use the templated encode() (same method pasted above)? > Source/WebKit/Platform/IPC/Encoder.h:80 > + // FIXME: Check that this is a valid enum value. Here, too. >> Source/WebKit/Platform/IPC/Encoder.h:93 >> + 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? Clearly it's `void`. (There are a lot of places where I think `auto` is not clear, but I keep my mouth shut because I don't care enough to spend time discussing it.) Happy to change this one, though. Should I change this one as well? It has the same issue. template<typename E> auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value> { ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value))); encode(static_cast<typename std::underlying_type<E>::type>(value)); }
Comment on attachment 399299 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=399299&action=review >>> 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. > > Sure, I can change this. I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it. We should also look for places where we just cast enum values to integral types for IPC. There are still a few of them around from the good ol' days. >>> 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. > > It's more about catching our own mistakes. This method has a Debug assertion: > > template<typename E> > auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value> > { > ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value))); > encode(static_cast<typename std::underlying_type<E>::type>(value)); > } > > I assume that's okay to add? So change "Check" to "Add Debug assert"? > > Do we eventually want to get rid of encodeEnum() (just like decodeEnum()) and just use the templated encode() (same method pasted above)? Debug assertion is fine. >>> Source/WebKit/Platform/IPC/Encoder.h:93 >>> + 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? > > Clearly it's `void`. (There are a lot of places where I think `auto` is not clear, but I keep my mouth shut because I don't care enough to spend time discussing it.) Happy to change this one, though. > > Should I change this one as well? It has the same issue. > > template<typename E> > auto encode(E value) -> std::enable_if_t<std::is_enum<E>::value> > { > ASSERT(WTF::isValidEnum<E>(static_cast<typename std::underlying_type<E>::type>(value))); > encode(static_cast<typename std::underlying_type<E>::type>(value)); > } Yes, please change them both. A template parameter indicating what the template is used for is much better than a mess in the return type, especially with an implicit return type.
Comment on attachment 399299 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=399299&action=review >>> 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. > > Sure, I can change this. I wanted to use WTF::isValidEnum<EnumType>() to check the type, and I assume that would have lead me down the enum class/EnumTraits path, but I didn't know how to say it. Should this method also have a FIXME to remove it? If not, I don't understand why. template<typename E, std::enable_if_t<std::is_enum<E>::value>* = nullptr> WARN_UNUSED_RETURN bool decode(E& enumValue) { typename std::underlying_type<E>::type value; if (!decode(value)) return false; if (!WTF::isValidEnum<E>(value)) return false; enumValue = static_cast<E>(value); return true; }
We should remove decodeEnum and rename all of them to decode, and also be pushing towards using operator>> Same with encodeEnum, which is super easy to replace with operator<<
(In reply to Alex Christensen from comment #6) > We should remove decodeEnum and rename all of them to decode, and also be > pushing towards using operator>> > Same with encodeEnum, which is super easy to replace with operator<< Okay, got it. Thanks!
Created attachment 399316 [details] Patch for landing
Committed r261672: <https://trac.webkit.org/changeset/261672> All reviewed patches have been landed. Closing bug and clearing flags on attachment 399316 [details].