[webkit-reviews] review granted: [Bug 220365] [JSC] Allow to build WebAssembly without B3 : [Attachment 418027] Make Wasm/B3 a compile-time option, v6

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 220365: [JSC] Allow to build WebAssembly without B3
https://bugs.webkit.org/show_bug.cgi?id=220365

Attachment 418027: Make Wasm/B3 a compile-time option, v6

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




--- Comment #12 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 418027
  --> https://bugs.webkit.org/attachment.cgi?id=418027
Make Wasm/B3 a compile-time option, v6

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

r=me with improvements.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

I think you can make this just `ENABLE(WEBASSEMBLY_B3JIT)`. 
`ENABLE(WEBASSEMBLY_B3JIT)` should imply `ENABLE(WEBASSEMBLY)`.  Otherwise,
it's pointless.

Please also update the comment at the matching `#endif` below with
`ENABLE(WEBASSEMBLY_B3JIT)` instead of `ENABLE(WEBASSEMBLY_B3JIT)`.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.h:28
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

Ditto here and in other files below.  Also, please update all the corresponding
`#endif`s.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:29
> +#if ENABLE(WEBASSEMBLY) && ENABLE(WEBASSEMBLY_B3JIT)

As sanity check, please add the following after the #includes below:

#if !ENABLE(WEBASSEMBLY)
#error ENABLE(WEBASSEMBLY_B3JIT) is enabled, but ENABLE(WEBASSEMBLY) is not.
#endif

This ensures that we'll get a build error if anyone ever misconfigures these
flags.	We'll probably get a build error anyway due to use of Wasm data
structures, but having this #error will make it more explicit, plus it
documents this invariant explicitly.  At first, I thought to put this in
PlatformEnable.h, but reconsidered and decided that we don't really need to do
this check repeatedly.

> Source/JavaScriptCore/wasm/WasmCallee.h:138
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCallee.h:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:77
> +#if ENABLE(WEBASSEMBLY_B3JIT)

nit: let's move this `#if` above the `} else {` since the entire else clause if
not needed otherwise.

> Source/JavaScriptCore/wasm/WasmCodeBlock.cpp:106
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmLLIntGenerator.h:32
> +#include "WasmModuleInformation.h"
> +#include "WasmSignature.h"

Let's just forward declare these like we do with the FunctionCodeBlock class
below.	It's better to reduce the amount of #include load unless we really need
it.

> Source/JavaScriptCore/wasm/WasmOperations.cpp:457
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text is definitely long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmPlan.cpp:191
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:258
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.

> Source/JavaScriptCore/wasm/WasmThunks.cpp:99
> +#endif

nit: please add `// ENABLE(WEBASSEMBLY_B3JIT)` after the #endif.  This body of
text seems long enough that this is warranted.