Re: [Tarantool-patches] [PATCH 3/5] build: add clang-format rules

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

See 4 comments below.

On 30.10.2020 12:43, Kirill Yukhin wrote:
> This patch introduces support of `make format-set`
> and `make format-check` commands which invoes Clang

1. invoes -> invokes.

> v'11 formatter.> ---
>  CMakeLists.txt | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 512f50e..cd72e15 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -20,11 +20,13 @@ include(FindPackageMessage)
>  include(ExternalProject)
>  
>  find_program(ECHO echo)
> -find_program(CAT cat)
>  find_program(BASH bash)
> +find_program(CAT cat)
> +find_program(CLANG_FORMAT clang-format-11)
> +find_program(CTAGS ctags)
> +find_program(FIND find)
>  find_program(GIT git)
>  find_program(LD ld)
> -find_program(CTAGS ctags)

2. Why did you move ctags and cat?

>  find_program(LUACHECK luacheck ENV PATH)
>  
>  # Define PACKAGE macro in tarantool/config.h
> @@ -164,6 +166,38 @@ add_custom_command(TARGET luacheck
>      COMMENT "Perform static analysis of Lua code"
>  )
>  
> +if(CLANG_FORMAT)

3. In this file all 'if' have a whitespace before condition. Please,
follow this style.

> +    #
> +    # Enable 'make format-set' target.
> +    #
> +    add_custom_target(format-set
> +	COMMAND ${FIND} "${PROJECT_SOURCE_DIR}/src/box"
> +	        -iname "*.h" -o -iname "*.c" -o -iname "*.cc" |grep -v sql
> +		|xargs ${CLANG_FORMAT} -i
> +	COMMENT "Perform code style update w/ clang-format over code base"
> +    )
> +
> +    #
> +    # Enable 'make format-check' target.
> +    #
> +    add_custom_target(format-check
> +	COMMAND ${FIND} "${PROJECT_SOURCE_DIR}/src/box"
> +                -iname "*.h" -o -iname "*.c" -o -iname "*.cc"
> +	        |grep -v sql
> +	        |xargs ${CLANG_FORMAT} -output-replacements-xml
> +	        |tee
> +	        ${CMAKE_BINARY_DIR}/check_clang_format_file.txt |
> +                grep -c "replacement " | tr -d "[:cntrl:]" && echo
> +                " replacements necessary"
> +        COMMAND ! grep -c "replacement "
> +                ${CMAKE_BINARY_DIR}/check_clang_format_file.txt >
> +                /dev/null
> +	COMMENT "Check code style w/ clang-format over code base"
> +    )
> +else()
> +    message(WARNING "Clang formatter v11 (clang-format-11) wasn't found")

4. On Mac the command is called clang-format. And I don't want to mess with
the paths to make it called clang-format-11. Please, look for the executable
properly and use 'clang-format --version' to detect the version, and fail if
it is less than 11 (in case you want not less than 11).

And while we are here, I ask you to provide more info in the comments why
did you choose version 11, and not an older version, as minimal supported.

Currently I don't see a single word not in the commit message nor in the
comments.

> +endif()
> +
>  if (WITH_JEPSEN)
>      ExternalProject_Add(
>          jepsen-tests
>