[webkit-reviews] review granted: [Bug 212230] Use an OptionSet instead of uint8_t for MessageFlags : [Attachment 399979] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 212230: Use an OptionSet instead of uint8_t for MessageFlags
https://bugs.webkit.org/show_bug.cgi?id=212230

Attachment 399979: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 399979
  --> https://bugs.webkit.org/attachment.cgi?id=399979
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::DispatchMessageWhenWaitingForUnboundedSyncRepl
y);

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.