From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7457D446439 for ; Sun, 8 Nov 2020 18:09:13 +0300 (MSK) From: Vladislav Shpilevoy References: Message-ID: Date: Sun, 8 Nov 2020 16:09:11 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 3/5] build: add clang-format rules List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirill Yukhin , tarantool-patches@dev.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 >