Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit

[Alexander Turenko <alexander.turenko@tarantool.org>]

LGTM.

There are several style comments: feel free to fix them or ignore. No
need to re-review with me.

WBR, Alexander Turenko.

> Branch: OKriw/gh-3743-LuaJIT-does-not-use-LTO-with-DENABLE_LTO=ON-full-ci

Pasted parts of actual patch comment it.

> diff --git a/cmake/luajit.cmake b/cmake/luajit.cmake
> index 10df633d5..555bc8371 100644
> --- a/cmake/luajit.cmake
> +++ b/cmake/luajit.cmake
> @@ -230,6 +230,16 @@ macro(luajit_build)
>      # above.
>      set (luajit_ld ${CMAKE_LINKER})
>      set (luajit_ar ${CMAKE_AR} rcus)
> +    # Enablibg LTO for luajit if DENABLE_LTO set.

Typo: Enablibg.

Nit: I would move it below `set (luajit_strip ${CMAKE_STRIP})` line and
format it as a separate block (add an empty line before it). Just for
readability.

> +    if (ENABLE_LTO)
> +        message(STATUS "Enable LTO for luajit")
> +        set (luajit_ldflags ${luajit_ldflags} ${LDFLAGS_LTO})
> +        message(STATUS "ld: " ${luajit_ldflags})
> +        set (luajit_cflags ${luajit_cflags} ${CFLAGS_LTO})
> +        message(STATUS "cflags: " ${luajit_cflags})
> +        set (luajit_ar  ${AR_LTO} rcus)
> +        message(STATUS "ar: " ${luajit_ar})
> +    endif()

Nit: Those variables are lists and they are printed without spaces
(I don't know exact reason):

 | -- Enable LTO for luajit
 | -- ld: -Wno-lto-type-mismatch-Wno-lto-type-mismatch
 | -- cflags: -Wno-unknown-pragmas-fexceptions-funwind-tables-fno-omit-frame-pointer-fno-stack-protector-fno-common-msse2-Wno-parentheses-equality-Wno-tautological-compare-Wno-varargs-Wno-implicit-fallthrough-flto=thin
 | -- ar: "/usr/lib/llvm-8/bin/llvm-ar"rcus

You should include them into quotes, like so:

 | -message(STATUS "ld: " ${luajit_ldflags})
 | +message(STATUS "ld: ${luajit_ldflags}")

(BTW, it is not ld executable, but ldflags; so I would name it 'ldflags'
in the output.)

Nit: LTO flags are already printed before:

 | -- ld version string: GNU ld (GNU Binutils for Debian) 2.31.1
 | -- Found ld.bfd version: 2.31
 | -- CFLAGS_LTO: -flto=thin
 | -- LDFLAGS_LTO: -Wno-lto-type-mismatch
 | -- AR_LTO: "/usr/lib/llvm-8/bin/llvm-ar"
 | -- Enabling LTO: TRUE

But I don't have strict objection against printing it again.