Hi, Max On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote: > Hi, Sergey! > Please consider my comments below. > > From: Sergey Bronnikov > > > In Tarantool we use our own fork of checkpatch [1] with > additional check > types. It's logical to use it in LuaJIT development. However, > we don't > need to enable all checks [2] implemented in checkpatch, > therefore a > number of checks are disabled. > > Patch introduces two new CMake targets: "LuaJIT-checkpatch", > that checks > patches on top of the master branch using script > checkpatch.pl, and > > Typo: s/using/using the/ > Fixed. > target "check", that combines LuaJIT-luacheck and > LuaJIT-checkpatch. By > default CMake looking for checkpatch.pl in a directory > "checkpatch" in > > Typo: s/looking/looks/ > Typo: s/in a directory/in the directory/ > Fixed. > > LuaJIT repository root directory and in a directories > specified in PATH. > > Typo: s/LuaJIT/the LuaJIT/ > Typo: s/a directories/directories/ > Fixed. > > 1. https://github.com/tarantool/checkpatch > 2. > https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst > --- >  test/CMakeLists.txt | 51 > +++++++++++++++++++++++++++++++++++++++++++++ >  1 file changed, 51 insertions(+) > > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt > index 47296a22..5ec0bed6 100644 > --- a/test/CMakeLists.txt > +++ b/test/CMakeLists.txt > @@ -42,6 +42,56 @@ else() >    ) >  endif() > > +find_program(CHECKPATCH checkpatch.pl > + HINTS ${PROJECT_SOURCE_DIR}/checkpatch) > +add_custom_target(${PROJECT_NAME}-checkpatch) > +set(MASTER_BRANCH "tarantool/master") > +if(CHECKPATCH) > + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch > + COMMENT "Running checkpatch" > + COMMAND > + ${CHECKPATCH} > + # Description of supported checks in > + # > https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst > + --codespell > + --color=always > + --git ${MASTER_BRANCH}..HEAD > + --show-types > + --ignore BAD_SIGN_OFF > + --ignore BLOCK_COMMENT_STYLE > + --ignore CODE_INDENT > + --ignore COMMIT_LOG_LONG_LINE > + # Requires at least two lines in commit message and this > + # is annoying. > + --ignore COMMIT_MESSAGE > + --ignore CONSTANT_COMPARISON > + --ignore FUNCTION_NAME_NO_NEWLINE > + --ignore GIT_COMMIT_ID > + --ignore INCLUDE_GUARD > + --ignore LOGICAL_CONTINUATIONS > + --ignore LONG_LINE > + --ignore NO_CHANGELOG > + --ignore NO_DOC > + --ignore NO_TEST > + --ignore PREFER_DEFINED_ATTRIBUTE_MACRO > + --ignore SPACING > + --ignore SUSPECT_CODE_INDENT > + --ignore TABSTOP > + --ignore TRAILING_STATEMENTS > + --ignore UNCOMMENTED_DEFINITION > + --ignore UNSAFE_FUNCTION > + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} > + ) > +else() > > I suggest doing the same as with coverage — move that logic into a > separate module, > so the test/CMakeLists.txt remains readable and clean. > Done, force-pushed. commit 609f893ecc5ee9895916c637c145489fe59d9bb0 Author: Sergey Bronnikov Date:   Fri Aug 4 13:51:23 2023 +0300     cmake: checkpatch module diff --git a/CMakeLists.txt b/CMakeLists.txt index 0204e852..b1a442b7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,9 +29,12 @@ endif()  set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") +set(GIT_MASTER_BRANCH "tarantool/master") +  include(LuaJITUtils)  include(SetBuildParallelLevel)  include(SetVersion) +include(CheckPatch)  # --- Variables to be exported to child scopes --------------------------------- diff --git a/cmake/CheckPatch.cmake b/cmake/CheckPatch.cmake new file mode 100644 index 00000000..243ee426 --- /dev/null +++ b/cmake/CheckPatch.cmake @@ -0,0 +1,46 @@ +find_program(CHECKPATCH checkpatch.pl +             HINTS ${PROJECT_SOURCE_DIR}/checkpatch) +add_custom_target(${PROJECT_NAME}-checkpatch) +if(CHECKPATCH) +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch +    COMMENT "Running checkpatch" +    COMMAND +      ${CHECKPATCH} +        # Description of supported checks in +        # https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst +        --codespell +        --color=always +        --git ${GIT_MASTER_BRANCH}..HEAD +        --show-types +        --ignore BAD_SIGN_OFF +        --ignore BLOCK_COMMENT_STYLE +        --ignore CODE_INDENT +        --ignore COMMIT_LOG_LONG_LINE +        # Requires at least two lines in commit message and this +        # is annoying. +        --ignore COMMIT_MESSAGE +        --ignore CONSTANT_COMPARISON +        --ignore FUNCTION_NAME_NO_NEWLINE +        --ignore GIT_COMMIT_ID +        --ignore INCLUDE_GUARD +        --ignore LOGICAL_CONTINUATIONS +        --ignore LONG_LINE +        --ignore NO_CHANGELOG +        --ignore NO_DOC +        --ignore NO_TEST +        --ignore PREFER_DEFINED_ATTRIBUTE_MACRO +        --ignore SPACING +        --ignore SUSPECT_CODE_INDENT +        --ignore TABSTOP +        --ignore TRAILING_STATEMENTS +        --ignore UNCOMMENTED_DEFINITION +        --ignore UNSAFE_FUNCTION +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} +  ) +else() +  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch target is dummy") +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG} +    COMMENT ${MSG} +  ) +endif() diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 5ec0bed6..66f6ee77 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -42,52 +42,6 @@ else()    )  endif() -find_program(CHECKPATCH checkpatch.pl -             HINTS ${PROJECT_SOURCE_DIR}/checkpatch) -add_custom_target(${PROJECT_NAME}-checkpatch) -set(MASTER_BRANCH "tarantool/master") -if(CHECKPATCH) -  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch -    COMMENT "Running checkpatch" -    COMMAND -      ${CHECKPATCH} -        # Description of supported checks in -        # https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst -        --codespell -        --color=always -        --git ${MASTER_BRANCH}..HEAD -        --show-types -        --ignore BAD_SIGN_OFF -        --ignore BLOCK_COMMENT_STYLE -        --ignore CODE_INDENT -        --ignore COMMIT_LOG_LONG_LINE -        # Requires at least two lines in commit message and this -        # is annoying. -        --ignore COMMIT_MESSAGE -        --ignore CONSTANT_COMPARISON -        --ignore FUNCTION_NAME_NO_NEWLINE -        --ignore GIT_COMMIT_ID -        --ignore INCLUDE_GUARD -        --ignore LOGICAL_CONTINUATIONS -        --ignore LONG_LINE -        --ignore NO_CHANGELOG -        --ignore NO_DOC -        --ignore NO_TEST -        --ignore PREFER_DEFINED_ATTRIBUTE_MACRO -        --ignore SPACING -        --ignore SUSPECT_CODE_INDENT -        --ignore TABSTOP -        --ignore TRAILING_STATEMENTS -        --ignore UNCOMMENTED_DEFINITION -        --ignore UNSAFE_FUNCTION -    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} -  ) -else() -  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  ) > + 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 > +) > + >  set(LUAJIT_TEST_COMMAND "${LUAJIT_TEST_BINARY} -e > dofile[[${LUAJIT_TEST_INIT}]]") >  separate_arguments(LUAJIT_TEST_COMMAND) > > @@ -75,5 +125,6 @@ if(LUAJIT_USE_TEST) >    add_custom_target(test DEPENDS >      ${PROJECT_NAME}-test >      ${PROJECT_NAME}-luacheck > + ${PROJECT_NAME}-checkpatch >    ) >  endif() > -- > 2.34.1 > > -- > Best regards, > Maxim Kokryashkin >