[webkit-reviews] review denied: [Bug 188476] [CMake][GTK] Disable JIT on IA-32 without SSE2 : [Attachment 346916] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied
karogyoker2+webkit at gmail.com's request for review:
Bug 188476: [CMake][GTK] Disable JIT on IA-32 without SSE2
https://bugs.webkit.org/show_bug.cgi?id=188476

Attachment 346916: Patch

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




--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 346916
  --> https://bugs.webkit.org/attachment.cgi?id=346916
Patch

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

> Source/cmake/OptionsGTK.cmake:-129
> -WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ON)

Hm, I would rather keep this option PUBLIC. Can you change it to reuse
ENABLE_JIT_DEFAULT, like this:

WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_JIT PUBLIC ${ENABLE_JIT_DEFAULT})

That will be fragile, since it would be broken if the variable is removed from
WebKitFeatures.cmake, but I don't see a better option, so also add a warning
comment in WebKitFeatures.cmake to indicate the variable is surprisingly used
in OptionsGTK.cmake.

Then test that it actually works, of course.

> Source/cmake/WebKitFeatures.cmake:75
> -    if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR
WTF_CPU_X86)
> +    if (WTF_CPU_ARM OR WTF_CPU_ARM64 OR WTF_CPU_MIPS OR WTF_CPU_X86_64 OR
WTF_CPU_X86_SSE2)

This is never set in CMake, so you're disabling it for everyone. Look in the
toplevel CMakeLists.txt where WTF_CPU_X86 is set. You'll need to add some test
for SSE2 instruction set there to ensure it gets defined when appropriate.