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

[Alexander Turenko <alexander.turenko@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.