From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (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 6766042F4AD for ; Tue, 16 Jun 2020 21:32:06 +0300 (MSK) Date: Tue, 16 Jun 2020 21:31:20 +0300 From: Alexander Turenko Message-ID: <20200616183120.3dbhchwynjpv7fdp@tkn_work_nb> References: <20200312100549.31608-1-arkholga@tarantool.org> <20200616010232.3j2zvwo7z6lmnody@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH] cmake: add LTO support for building luajit List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Olga Arkhangelskaia Cc: tarantool-patches@dev.tarantool.org My review is not sufficient anymore, since I wrote some parts of the code. Sergey, Timur, can you participate here? > Minimum compiler/linker versions: clang 3.4, gcc 5.0+, ld 2.27+ due > errors and slow work. It seems it is not actual anymore (at least for ld). If you want to highlight minimal requirements, I think it would be better to mention f9e28ce4602aff3f9bb4e743b0d6167b0f8df88d commit. Typo: Enablibg. > +    if (${ENABLE_LTO}) No need to use ${var} inside if condition. See the surrounding code. > +`......message(STATUS "Enablig LTO for luajit") > +        set (luajit_ld ${CMAKE_C_LINK_OPTIONS_IPO}) The variable is reassigned right below (it is why the build does not fail). It seems, you should find a way to verify that your changes actually enable LTO for luajit. Either by looking into actual compiler / linker options, or by looking into compiled code differences (say, find a cheap function in one compilation unit and verify that it is inlined into another one). It uses CMAKE_C_LINK_OPTIONS_IPO internal variable directly and ignores LDFLAGS_LTO that is set by lto.cmake. I suggest to use LDFLAGS_LTO. I guess you want to set luajit_ldflags, not luajit_ld. Use spaces for indent in CMake and Lua files. I guess all modern text editors may be properly configured to have different indent symbol / level for different file types. NB: I suggest to rebase your patchsets on top of master on each review iteration. ---- I know, it is really hard to read the same code again and again when everything looks finished. But it is inevitable part of any development process due to the human nature: we do mistakes and should build processes that allow us to catch them. I suggest to introduce a personal self-review checklist that will grow with review comments from teammates.