From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 6DA58445320
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  8 Jul 2020 15:24:44 +0300 (MSK)
Date: Wed, 8 Jul 2020 15:23:57 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20200708122357.koubasrbblpul3yw@tkn_work_nb>
References: <20200312100549.31608-1-arkholga@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200312100549.31608-1-arkholga@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building
	luajit
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Olga Arkhangelskaia <arkholga@tarantool.org>
Cc: tarantool-patches@dev.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.