[webkit-reviews] review granted: [Bug 206563] Shrink BytecodeStructs.h : [Attachment 388387] Patch

[bugzilla-daemon at webkit.org]

Tadeu Zagallo <tzagallo at apple.com> has granted Robin Morisset
<rmorisset at apple.com>'s request for review:
Bug 206563: Shrink BytecodeStructs.h
https://bugs.webkit.org/show_bug.cgi?id=206563

Attachment 388387: Patch

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




--- Comment #3 from Tadeu Zagallo <tzagallo at apple.com> ---
Comment on attachment 388387
  --> https://bugs.webkit.org/attachment.cgi?id=388387
Patch

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

Looks good to me, just a couple very minor comments.

> Source/JavaScriptCore/ChangeLog:13
> +	   - The checks that op_wide16, op_wide32, wasm_wide16, wasm_wide32 fit
in OpcodeID/WasmOpcodeID are now in a single place as a static assert in
Fits.h, instead of being repeated in the checkImpl of every single instruction

Nice!

> Source/JavaScriptCore/generator/Opcode.rb:159
> +    def baseClass

nit: in ruby we usually use snake_case

> Source/JavaScriptCore/generator/OpcodeGroup.rb:59
> +    def constructors

Would it make sense to share this code with Opcode.rb?

> Source/JavaScriptCore/generator/Section.rb:46
> +  def create_opcode(name, config, opcode_group)

You can also use a default argument, e.g. `opcode_group = nil`