[webkit-reviews] review granted: [Bug 190758] Re-enable LTO support : [Attachment 365450] Removed support for LTO on ARM. Removed 32-bit support. Enable only when using Xcode 10.2 or later.

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Keith Rollin <krollin at apple.com>'s
request for review:
Bug 190758: Re-enable LTO support
https://bugs.webkit.org/show_bug.cgi?id=190758

Attachment 365450: Removed support for LTO on ARM. Removed 32-bit support.
Enable only when using Xcode 10.2 or later.

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




--- Comment #21 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 365450
  --> https://bugs.webkit.org/attachment.cgi?id=365450
Removed support for LTO on ARM. Removed 32-bit support. Enable only when using
Xcode 10.2 or later.

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

Ok as-s. No need to address comments, though addressing them would make this
patch better in my opinion. cq- for you to contemplate. My comments apply to
each file, including ChangeLogs in this patch..

> Source/ThirdParty/ANGLE/ChangeLog:24
> +	   - If building with `build-root`, specify --lto={non,thin,full} on
the

non

> Source/JavaScriptCore/Configurations/Base.xcconfig:182
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

macos

missing period at the end. <> around URL

> Source/JavaScriptCore/Configurations/Base.xcconfig:192
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 =
$(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Could be ordered based on increasing major version.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:84
> +// Disable on all platforms other than macos, due to rdar://problem/49013399

See comments above.

> Source/ThirdParty/ANGLE/Configurations/Base.xcconfig:93
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1010 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000_1000 = YES;
> +WK_XCODE_VERSION_BEFORE_10_2_1000 =
$(WK_XCODE_VERSION_BEFORE_10_2_1000_$(XCODE_VERSION_MINOR));

Ok as-is. Same ordering suggestion as above.