Hi, Sergey! Please consider my comments below.   >  >>From: Sergey Bronnikov < sergeyb@tarantool.org > >> >>In Tarantool we use our own fork of checkpatch [2] with additional check >>types. It's logical to use it in a LuaJIT development. We don't need >Typo: s/in a/in/ >>check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and >>others, so to be able to customize command-line options Github Action, provided >>by checkpatch repository [3], was added to the repository. >Typo: s/by checkpatch/by the checkpatch/ >> >>See documentation for used checkpatch in [4]. >Typo: s/documentation/the documentation/ >Typo: s/for used/for the/ >> >>Patch introduce new CMake targets: LuaJIT-checkpatch, that checks >Typo: s/introduce/introduces/ >>patches on top of master branch using script checkpatch.pl [1], and >Typo: s/on top of/on top of the/ >>target check, that combines LuaJIT-luacheck and LuaJIT-checkpatch. >> >>1. https://docs.kernel.org/dev-tools/checkpatch.html >>2. https://github.com/tarantool/checkpatch >>3. https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml >>4. https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst >Nit: It is kinda strange to see link [1] going after the link [4] in the commit message. >I think, it generally looks clearer, when they are ordered, but that’s a matter of taste. >Feel free to ignore. >> >>--- test/CMakeLists.txt | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >>diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt >>index 47296a22..ccbad035 100644 >>--- a/test/CMakeLists.txt >>+++ b/test/CMakeLists.txt >>@@ -42,6 +42,39 @@ else() >>   ) >> endif() >>  >>+find_program(CHECKPATCH checkpatch.pl >>+ HINTS ${PROJECT_SOURCE_DIR}/checkpatch) >>+if(CHECKPATCH) >I don’t really like that `MASTER_BRANCH` name is hardcoded. I think >it’s possible to implement it similarly to how it’s done in the `tarantool/checkpatch` >github action[1] with revision range. Or, at least, it is for sure possible to obtain >the master branch name dynamically. >>+ set(MASTER_BRANCH "tarantool/master") >>+ add_custom_target(${PROJECT_NAME}-checkpatch) >>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch >>+ COMMENT "Running checkpatch" >>+ COMMAND >>+ ${CHECKPATCH} >>+ --codespell >>+ --color=always >>+ --git ${MASTER_BRANCH}..HEAD >>+ --ignore COMMIT_LOG_LONG_LINE >>+ # Requires at least two lines in commit message and this >>+ # is annoying. >>+ --ignore COMMIT_MESSAGE >>+ --ignore NO_CHANGELOG >>+ --ignore NO_DOC >>+ --ignore NO_TEST >>+ --show-types >>+ WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} >>+ ) >>+else() >>+ add_custom_target(${PROJECT_NAME}-checkpatch) >It seems like the target definition can be moved out of the `if` statement >just before it. >>+ add_custom_command(TARGET ${PROJECT_NAME}-checkpatch >>+ COMMENT "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy" >>+ ) >>+endif() >>+ >>+add_custom_target(check >>+ DEPENDS ${PROJECT_NAME}-checkpatch ${PROJECT_NAME}-luacheck >>+) >>+ >As I have already said offline, I think we should include the `check` target as a dependency to the `test` target, just like it is currently done for the luacheck. It is much more convenient for local testing that way. >> set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e dofile[[${LUAJIT_TEST_INIT}]]") >> separate_arguments(LUAJIT_TEST_COMMAND) >>  >>-- >>2.34.1 >[1]:  https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml >-- >Best regards, >Maxim Kokryashkin