<HTML><BODY><div>Hi, Sergey!</div><div>Please consider my comments below.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16909666541899939236_BODY">From: Sergey Bronnikov <<a href="/compose?To=sergeyb@tarantool.org">sergeyb@tarantool.org</a>><br><br>In Tarantool we use our own fork of checkpatch [1] with additional check<br>types. It's logical to use it in LuaJIT development. However, we don't<br>need to enable all checks [2] implemented in checkpatch, therefore a<br>number of checks are disabled.<br><br>Patch introduces two new CMake targets: "LuaJIT-checkpatch", that checks<br>patches on top of the master branch using script checkpatch.pl, and</div></div></div></div></blockquote><div>Typo: s/using/using the/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>target "check", that combines LuaJIT-luacheck and LuaJIT-checkpatch. By<br>default CMake looking for checkpatch.pl in a directory "checkpatch" in</div></div></div></div></blockquote><div>Typo: s/looking/looks/</div><div>Typo: s/in a directory/in the directory/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>LuaJIT repository root directory and in a directories specified in PATH.</div></div></div></div></blockquote><div>Typo: s/LuaJIT/the LuaJIT/</div><div>Typo: s/a directories/directories/</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div><br>1. <a href="https://github.com/tarantool/checkpatch" target="_blank">https://github.com/tarantool/checkpatch</a><br>2. <a href="https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst" target="_blank">https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst</a><br>---<br> test/CMakeLists.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++<br> 1 file changed, 51 insertions(+)<br><br>diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt<br>index 47296a22..5ec0bed6 100644<br>--- a/test/CMakeLists.txt<br>+++ b/test/CMakeLists.txt<br>@@ -42,6 +42,56 @@ else()<br> )<br> endif()<br> <br>+find_program(CHECKPATCH checkpatch.pl<br>+ HINTS ${PROJECT_SOURCE_DIR}/checkpatch)<br>+add_custom_target(${PROJECT_NAME}-checkpatch)<br>+set(MASTER_BRANCH "tarantool/master")<br>+if(CHECKPATCH)<br>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch<br>+ COMMENT "Running checkpatch"<br>+ COMMAND<br>+ ${CHECKPATCH}<br>+ # Description of supported checks in<br>+ # <a href="https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst" target="_blank">https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst</a><br>+ --codespell<br>+ --color=always<br>+ --git ${MASTER_BRANCH}..HEAD<br>+ --show-types<br>+ --ignore BAD_SIGN_OFF<br>+ --ignore BLOCK_COMMENT_STYLE<br>+ --ignore CODE_INDENT<br>+ --ignore COMMIT_LOG_LONG_LINE<br>+ # Requires at least two lines in commit message and this<br>+ # is annoying.<br>+ --ignore COMMIT_MESSAGE<br>+ --ignore CONSTANT_COMPARISON<br>+ --ignore FUNCTION_NAME_NO_NEWLINE<br>+ --ignore GIT_COMMIT_ID<br>+ --ignore INCLUDE_GUARD<br>+ --ignore LOGICAL_CONTINUATIONS<br>+ --ignore LONG_LINE<br>+ --ignore NO_CHANGELOG<br>+ --ignore NO_DOC<br>+ --ignore NO_TEST<br>+ --ignore PREFER_DEFINED_ATTRIBUTE_MACRO<br>+ --ignore SPACING<br>+ --ignore SUSPECT_CODE_INDENT<br>+ --ignore TABSTOP<br>+ --ignore TRAILING_STATEMENTS<br>+ --ignore UNCOMMENTED_DEFINITION<br>+ --ignore UNSAFE_FUNCTION<br>+ WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}<br>+ )<br>+else()</div></div></div></div></blockquote><div>I suggest doing the same as with coverage — move that logic into a separate module,</div><div>so the test/CMakeLists.txt remains readable and clean.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch<br>+ COMMENT "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy"<br>+ )<br>+endif()<br>+<br>+add_custom_target(check<br>+ DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck<br>+)<br>+<br> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]")<br> separate_arguments(LUAJIT_TEST_COMMAND)<br> <br>@@ -75,5 +125,6 @@ if(LUAJIT_USE_TEST)<br> add_custom_target(test DEPENDS<br> ${PROJECT_NAME}-test<br> ${PROJECT_NAME}-luacheck<br>+ ${PROJECT_NAME}-checkpatch<br> )<br> endif()<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>