<!DOCTYPE html>
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Sergey,</p>
<p>see my comments below</p>
<p><br>
</p>
<div class="moz-cite-prefix">On 3/21/24 18:00, Sergey Kaplun wrote:<br>
</div>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">Hi, Sergey!
Thanks for the patch!
Please consider the several minor suggestions below.
On 21.03.24, Sergey Bronnikov wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">From: Sergey Bronnikov <a class="moz-txt-link-rfc2396E" href="mailto:sergeyb@tarantool.org"><sergeyb@tarantool.org></a>
The patch updates CMake macro LibRealPath:
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/CMake/the CMake/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
- Replace CMAKE_CXX_COMPILER with CMAKE_C_COMPILER,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: This line isn't filled enough.</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> because LuaJIT doesn't require CXX compiler and thus
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/compiler/compiler,/</pre>
</blockquote>
Fixed.<br>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> it can be unavailable on a build system.
- Add a check for a code returned by a command with compiler.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/compiler/the compiler/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- Add a check for a value returned by compiler, the value equal to
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/compiler, the/the compiler. A/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> passed one means that library was not found in a system.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/passed one/a passed one/
Typo: s/library/the library/
Typo: s/a system/the system/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">---
test/LuaJIT-tests/CMakeLists.txt | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/test/LuaJIT-tests/CMakeLists.txt b/test/LuaJIT-tests/CMakeLists.txt
index a0fb5440..f744abfe 100644
--- a/test/LuaJIT-tests/CMakeLists.txt
+++ b/test/LuaJIT-tests/CMakeLists.txt
@@ -33,12 +33,27 @@ if(LUAJIT_USE_ASAN)
# <a class="moz-txt-link-freetext" href="https://github.com/google/sanitizers/issues/934">https://github.com/google/sanitizers/issues/934</a>.
macro(LibRealPath output lib)
execute_process(
- COMMAND ${CMAKE_CXX_COMPILER} -print-file-name=${lib}
+ # CMAKE_C_COMPILER is used because it has the same behaviour
+ # as CMAKE_CXX_COMPILER, but CMAKE_CXX_COMPILER is not required
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Comment length is more than 66 symbols.</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # for building LuaJIT and can be missed in GH Actions.
+ COMMAND ${CMAKE_C_COMPILER} -print-file-name=${lib}
OUTPUT_VARIABLE LIB_LINK
OUTPUT_STRIP_TRAILING_WHITESPACE
+ RESULT_VARIABLE RES
)
+ # GCC and Clang returns a passed filename,
+ # when library was not found.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Nit: Something strange with comment width. You can use more space from
the first line.</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (${LIB_LINK} STREQUAL ${lib})
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I suppose, it should be:
| if (LIB_LINK STREQUAL ${lib})
Or we may get the following error if the `LIB_LINK` isn't defined:
| cc: error: unrecognized command-line option '-not-an-option=libstdc++.so'
| cc: fatal error: no input files
| compilation terminated.
| CMake Error at test/LuaJIT-tests/CMakeLists.txt:42 (if):
| if given arguments:
</pre>
</blockquote>
<p><br>
</p>
<p>Fixed.</p>
<p>-- a/test/LuaJIT-tests/CMakeLists.txt<br>
+++ b/test/LuaJIT-tests/CMakeLists.txt<br>
@@ -40,7 +40,7 @@ if(LUAJIT_USE_ASAN)<br>
)<br>
# GCC and Clang returns a passed filename, when library<br>
# was not found.<br>
- if (${LIB_LINK} STREQUAL ${lib})<br>
+ if (LIB_LINK STREQUAL ${lib})<br>
message(FATAL_ERROR "Library '${lib}' is not found")<br>
endif()<br>
if (NOT RES EQUAL 0)<br>
<br>
</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ message(FATAL_ERROR "Library '${lib}' is not found")
+ endif()
+ if (NOT RES EQUAL 0)
+ message(FATAL_ERROR "Executing '${CMAKE_C_COMPILER} \
+ -print-file-name=${lib}' has failed")
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Please reformat this code like the following to be consistent with the
rest of the code base:
| message(FATAL_ERROR
| "Executing '${CMAKE_C_COMPILER} -print-file-name=${lib}' has failed"
| )
</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ endif()
+
# Fortunately, we are not interested in macOS here, so we can
- # use realpath.
+ # use realpath. Beware, builtin commands always returns
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/builtin/built-in/
Typo: s/returns/return/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
Also, why do you call it "built-in comands"? Maybe just name it
`realpath`.</pre>
</blockquote>
<p>because realpath is a <span class="HwtZe" lang="en"><span
class="jCAhz ChMk0b"><span class="ryNqvb">special case of
shell built-in</span></span></span></p>
<p><span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb">updated to realpath<br>
</span></span></span></p>
<p><span class="HwtZe" lang="en"><span class="jCAhz ChMk0b"><span
class="ryNqvb"></span></span></span></p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ # an exit code equal to 0, we cannot check is it failed or not.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Typo: s/, we/, so we/
Typo: s/is it failed/if it is failed/</pre>
</blockquote>
<p>Fixed.</p>
<p><br>
</p>
<blockquote type="cite" cite="mid:ZfxLmAlKKavUgoLV@root">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> execute_process(
COMMAND realpath ${LIB_LINK}
OUTPUT_VARIABLE ${output}
--
2.34.1
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>