Use an OptionSet instead of uint8_t for MessageFlags
Created attachment 399979 [details] Patch
<rdar://problem/63496543>
Comment on attachment 399979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399979&action=review > Source/WebKit/Platform/IPC/ArgumentCoders.h:58 > + encoder << (static_cast<typename OptionSet<T>::StorageType>(optionSet.toRaw())); I don’t think the static_cast is needed any more here. OptionSet::toRaw already returns this type. Could just be: encoder << optionSet.toRaw(); > Source/WebKit/Platform/IPC/Decoder.cpp:29 > +#include "ArgumentCoders.h" Don’t understand why this is now needed and wasn't before, but I assume there’s a reason. > Source/WebKit/Platform/IPC/Encoder.cpp:117 > - *buffer() |= DispatchMessageWhenWaitingForUnboundedSyncReply; > - *buffer() &= ~DispatchMessageWhenWaitingForSyncReply; > + messageFlags().remove(MessageFlags::DispatchMessageWhenWaitingForSyncReply); > + messageFlags().add(MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply); Above you kept the order the same, but here you changed the order. Any reason? > Source/WebKit/Platform/IPC/Encoder.cpp:179 > + static_assert(sizeof(OptionSet<MessageFlags>::StorageType) == sizeof(uint8_t), "Encoder uses the first byte of the buffer for message flags."); The use of sizeof(uint8_t) here is a bit confusing to me. Maybe just write 1 instead? > Source/WebKit/Platform/IPC/Encoder.h:40 > +enum class ShouldDispatchWhenWaitingForSyncReply : uint8_t; > +enum class MessageFlags : uint8_t; > enum class MessageName : uint16_t; Re-sort alphabetically? > Source/WebKit/Platform/IPC/MessageFlags.h:41 > -enum class ShouldDispatchWhenWaitingForSyncReply { No, Yes, YesDuringUnboundedIPC }; > +enum class ShouldDispatchWhenWaitingForSyncReply : uint8_t { > + No, > + Yes, > + YesDuringUnboundedIPC > +}; Adding uint8_t, definitely an improvement. Spreading this out over 5 lines, doesn’t seem as obviously an improvement to me.
Comment on attachment 399979 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=399979&action=review >> Source/WebKit/Platform/IPC/Decoder.cpp:29 >> +#include "ArgumentCoders.h" > > Don’t understand why this is now needed and wasn't before, but I assume there’s a reason. In code I didn't change, we encode m_messageFlags which now needs the OptionSet encoder because the type changed. >> Source/WebKit/Platform/IPC/Encoder.cpp:117 >> + messageFlags().add(MessageFlags::DispatchMessageWhenWaitingForUnboundedSyncReply); > > Above you kept the order the same, but here you changed the order. Any reason? I figured as long as I'm changing both lines, the order being consistent with case ShouldDispatchWhenWaitingForSyncReply::Yes is nicer. >> Source/WebKit/Platform/IPC/MessageFlags.h:41 >> +}; > > Adding uint8_t, definitely an improvement. Spreading this out over 5 lines, doesn’t seem as obviously an improvement to me. This makes the change history cleaner if we ever add another one.
http://trac.webkit.org/r262032