[webkit-reviews] review granted: [Bug 237347] Refactor OpcodeTraits to support the possibility of having 2-byte WASM opcode ids in bytecode streams : [Attachment 453625] Patch

[bugzilla-daemon at webkit.org]

Keith Miller <keith_miller at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 237347: Refactor OpcodeTraits to support the possibility of having 2-byte
WASM opcode ids in bytecode streams
https://bugs.webkit.org/show_bug.cgi?id=237347

Attachment 453625: Patch

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




--- Comment #9 from Keith Miller <keith_miller at apple.com> ---
Comment on attachment 453625
  --> https://bugs.webkit.org/attachment.cgi?id=453625
Patch

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

r=me with comments.

> Source/JavaScriptCore/bytecode/BytecodeDumper.h:-135
> -class BytecodeDumper final : public
JSC::BytecodeDumper<FunctionCodeBlockGenerator> {

How come we don't need this one anymore?

> Source/JavaScriptCore/bytecode/BytecodeDumper.h:151
> +    int outOfLineJumpOffset(JSInstructionStream::Offset) const override;

Why is this JSInstructionStream? I'm assuming it's just a copy paste error and
the two `Offset` types are the same in practice?

> Source/JavaScriptCore/bytecode/BytecodeGeneratorification.cpp:45
> +    typename JSInstructionStream::Offset point { 0 };

Do you need `typename` here? I thought that wasn't necessary as of C++20. Ditto
a bunch of other places.