[Tarantool-patches] [PATCH 0/5][v3] Fix typos and enable checkpatch

[Tarantool-patches] [PATCH 0/5][v3] Fix typos and enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Sometimes we do mistakes and typos. Reviewers spend a lot of
time in proofreading of comments, commit messages and code itself and
reports typos to authors. The idea is to automate a part of work made by reviewers
and highlight about all typos and code styles problems in continuous integration,
before sending patches to review.

Patch-series adds support of checkpatch to the build infrastructure
and Github Actions, and fix typos found by checkpatch and codespell.

Changelog v3:

- Addressed comments from Maxim Kokryashkin.
- Suppressed checkpatch warnings that not suitable for LuaJIT.

Changelog v2:

- Added CMake targets.
- Fixed typo in a step name.
- Fixed two typos.
- Updated Github Action for checkpatch, now checkpatch runs in workflow,
  not in action.
- Ignore check COMMIT_MESSAGE.

Branch: https://github.com/tarantool/luajit/tree/ligurio/gh-xxxx-spellchecking

Sergey Bronnikov (5):
  ci: fix a step name
  codehealth: fix typos
  cmake: introduce new targets
  ci: enable checkpatch
  test: fix codestyle

 .github/actions/checkpatch/action.yml         | 11 ++++
 .github/workflows/lint.yml                    | 22 +++++++-
 src/lj_sysprof.c                              |  2 +-
 src/lj_wbuf.h                                 |  2 +-
 src/luajit-gdb.py                             |  6 +--
 src/luajit_lldb.py                            |  6 +--
 test/CMakeLists.txt                           | 51 +++++++++++++++++++
 test/LuaJIT-tests/src/ctest.c                 |  2 +-
 test/tarantool-c-tests/CMakeLists.txt         |  1 -
 test/tarantool-c-tests/test.c                 |  2 +-
 test/tarantool-tests/CMakeLists.txt           |  2 +-
 test/tarantool-tests/fix-emit-rma.test.lua    |  2 +-
 .../gh-4199-gc64-fuse.test.lua                |  2 +-
 ...-6096-external-unwinding-on-arm64.test.lua |  2 +-
 test/tarantool-tests/gh-6163-min-max.test.lua |  2 +-
 .../lj-356-ir-khash-non-string-obj.test.lua   |  2 +-
 .../lj-416-xor-before-jcc.test.lua            |  2 +-
 ...6-arm64-incorrect-check-closed-uv.test.lua |  2 +-
 .../lj-512-profiler-hook-finalizers.test.lua  |  4 +-
 .../lj-603-err-snap-restore.test.lua          |  2 +-
 .../misclib-memprof-lapi.test.lua             |  6 +--
 test/tarantool-tests/tap.lua                  |  4 +-
 test/tarantool-tests/unit-jit-parse.test.lua  |  2 +-
 tools/memprof/parse.lua                       |  4 +-
 tools/memprof/process.lua                     |  4 +-
 25 files changed, 114 insertions(+), 33 deletions(-)
 create mode 100644 .github/actions/checkpatch/action.yml

-- 
2.34.1

[Tarantool-patches] [PATCH 1/5][v3] ci: fix a step name

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Fixed an incorrect name in a github actions step.
---
 .github/workflows/lint.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 70c98104..8154a622 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -48,6 +48,6 @@ jobs:
           echo CMAKE_BUILD_PARALLEL_LEVEL=$(($(nproc) + 1)) | tee -a $GITHUB_ENV
       - name: configure
         run: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja
-      - name: test
+      - name: luacheck
         run: cmake --build . --target LuaJIT-luacheck
         working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

[Tarantool-patches] [PATCH 2/5][v3] codehealth: fix typos

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Fix typos found with codespell in files with our own source code.
---
 src/lj_sysprof.c                                            | 2 +-
 src/lj_wbuf.h                                               | 2 +-
 src/luajit-gdb.py                                           | 6 +++---
 src/luajit_lldb.py                                          | 6 +++---
 test/LuaJIT-tests/src/ctest.c                               | 2 +-
 test/tarantool-tests/CMakeLists.txt                         | 2 +-
 test/tarantool-tests/fix-emit-rma.test.lua                  | 2 +-
 test/tarantool-tests/gh-4199-gc64-fuse.test.lua             | 2 +-
 .../gh-6096-external-unwinding-on-arm64.test.lua            | 2 +-
 test/tarantool-tests/gh-6163-min-max.test.lua               | 2 +-
 test/tarantool-tests/lj-416-xor-before-jcc.test.lua         | 2 +-
 .../lj-426-arm64-incorrect-check-closed-uv.test.lua         | 2 +-
 .../lj-512-profiler-hook-finalizers.test.lua                | 4 ++--
 test/tarantool-tests/lj-603-err-snap-restore.test.lua       | 2 +-
 test/tarantool-tests/misclib-memprof-lapi.test.lua          | 6 +++---
 test/tarantool-tests/tap.lua                                | 4 ++--
 test/tarantool-tests/unit-jit-parse.test.lua                | 2 +-
 tools/memprof/parse.lua                                     | 4 ++--
 tools/memprof/process.lua                                   | 4 ++--
 19 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
index 2e9ed9b3..745eb1a3 100644
--- a/src/lj_sysprof.c
+++ b/src/lj_sysprof.c
@@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L)
   if (SPS_HALT == sp->state) {
     errno = sp->saved_errno;
     sp->state = SPS_IDLE;
-    /* wbuf was terminated when error occured. */
+    /* wbuf was terminated when error occurred. */
     return PROFILE_ERRIO;
   }
 
diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
index 9eaa5e51..33ec8463 100644
--- a/src/lj_wbuf.h
+++ b/src/lj_wbuf.h
@@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n);
 /* Write string to the buffer. */
 void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s);
 
-/* Immediatly flush the buffer. */
+/* Immediately flush the buffer. */
 void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf);
 
 /* Check flags. */
diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
index 96ee2289..afdfecd9 100644
--- a/src/luajit-gdb.py
+++ b/src/luajit-gdb.py
@@ -268,7 +268,7 @@ def strdata(obj):
     try:
         return str(cast('char *', cast('GCstr *', obj) + 1))[len(PADDING):]
     except UnicodeEncodeError:
-        return "<luajit-gdb: error occured while rendering non-ascii slot>"
+        return "<luajit-gdb: error occurred while rendering non-ascii slot>"
 
 def itypemap(o):
     if LJ_64 and not LJ_GC64:
@@ -606,7 +606,7 @@ class LJDumpTable(LJBase):
     '''
 lj-tab <GCtab *>
 
-The command receives a GCtab adress and dumps the table contents:
+The command receives a GCtab address and dumps the table contents:
 * Metatable address whether the one is set
 * Array part <asize> slots:
   <aslot ptr>: [<index>]: <tv>
@@ -677,7 +677,7 @@ coroutine guest stack:
     + CP: Protected C frame
     + PP: VM performs a call as a result of executinig pcall or xpcall
 
-If L is ommited the main coroutine is used.
+If L is omitted the main coroutine is used.
     '''
 
     def invoke(self, arg, from_tty):
diff --git a/src/luajit_lldb.py b/src/luajit_lldb.py
index 9ee10269..4d7bef81 100644
--- a/src/luajit_lldb.py
+++ b/src/luajit_lldb.py
@@ -490,7 +490,7 @@ def strdata(obj):
         ptr = cast('char *', obj + 1)
         return ptr.summary
     except UnicodeEncodeError:
-        return "<luajit-lldb: error occured while rendering non-ascii slot>"
+        return "<luajit-lldb: error occurred while rendering non-ascii slot>"
 
 def itype(o):
     return tou32(o.it64 >> 47) if LJ_GC64 else o.it
@@ -909,7 +909,7 @@ class LJDumpTable(Command):
     '''
 lj-tab <GCtab *>
 
-The command receives a GCtab adress and dumps the table contents:
+The command receives a GCtab address and dumps the table contents:
 * Metatable address whether the one is set
 * Array part <asize> slots:
   <aslot ptr>: [<index>]: <tv>
@@ -979,7 +979,7 @@ coroutine guest stack:
     + CP: Protected C frame
     + PP: VM performs a call as a result of executinig pcall or xpcall
 
-If L is ommited the main coroutine is used.
+If L is omitted the main coroutine is used.
     '''
     def execute(self, debugger, args, result):
         l = self.parse(args)
diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/ctest.c
index d257567b..e99f2306 100644
--- a/test/LuaJIT-tests/src/ctest.c
+++ b/test/LuaJIT-tests/src/ctest.c
@@ -234,7 +234,7 @@ static int costatus(lua_State *L, lua_State *co) {
       else
 	return CO_SUS;  /* initial state */
     }
-    default:  /* some error occured */
+    default:  /* some error occurred */
       return CO_DEAD;
   }
 }
diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
index 6218f76a..f0bf8efb 100644
--- a/test/tarantool-tests/CMakeLists.txt
+++ b/test/tarantool-tests/CMakeLists.txt
@@ -132,7 +132,7 @@ else()
 endif()
 
 # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
-# with dependecies are set in scope of BuildTestLib macro.
+# with dependencies are set in scope of BuildTestLib macro.
 add_custom_target(tarantool-tests
   DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
 )
diff --git a/test/tarantool-tests/fix-emit-rma.test.lua b/test/tarantool-tests/fix-emit-rma.test.lua
index 42804447..cc97ff4d 100644
--- a/test/tarantool-tests/fix-emit-rma.test.lua
+++ b/test/tarantool-tests/fix-emit-rma.test.lua
@@ -4,7 +4,7 @@ local test = tap.test('fix-emit-rma'):skipcond({
   ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
 })
 
--- Need to test 2 cases of `emit_rma()` particulary on x64:
+-- Need to test 2 cases of `emit_rma()` particularly on x64:
 --   * `IR_LDEXP` with `fld` instruction for loading constant
 --      number `TValue` by address.
 --   * `IR_OBAR` with the corresponding `test` instruction on
diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
index ca050cf0..877f7884 100644
--- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
+++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
@@ -44,7 +44,7 @@ for n = 1, 100 do
       for i = 1, 5 do
         -- This constant fusion leads to the test failure.
         a[i] = 0
-        -- This summ is not necessarry but decreases the amount of
+        -- This summ is not necessary but decreases the amount of
         -- iterations.
         a[i] = a[i] + x + y
         -- Use all FPR registers and one value from the memory
diff --git a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
index 097e771c..00f58642 100644
--- a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
+++ b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
@@ -1,6 +1,6 @@
 local tap = require('tap')
 
--- Test file to check correctnes of external unwinding
+-- Test file to check correctness of external unwinding
 -- in LuaJIT.
 -- See also https://github.com/LuaJIT/LuaJIT/issues/698,
 -- https://github.com/LuaJIT/LuaJIT/pull/757.
diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
index 63437955..3b7a8e03 100644
--- a/test/tarantool-tests/gh-6163-min-max.test.lua
+++ b/test/tarantool-tests/gh-6163-min-max.test.lua
@@ -53,7 +53,7 @@ local x = 1
 
 jit.opt.start('hotloop=1')
 
--- XXX: Looping over the operations and their arguments breakes the
+-- XXX: Looping over the operations and their arguments breaks the
 -- semantics of some optimization tests below. The cases are
 -- copy-pasted to preserve optimization semantics.
 
diff --git a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
index 605bb92a..f71c9ee4 100644
--- a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
+++ b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
@@ -41,7 +41,7 @@ local testxor = ffi.load('libtestxor')
 -- pressure and specific registers allocations.
 local handler = setmetatable({}, {
   __newindex = function ()
-    -- 0 and nil are suggested as differnt constant-zero values
+    -- 0 and nil are suggested as different constant-zero values
     -- for the call and occupied different registers.
     testxor.test_xor_func(0, 0, 0, 0, 0, 0, nil, 0)
   end
diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
index a689ed60..ea01ba06 100644
--- a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
@@ -14,7 +14,7 @@ do
   -- The function's prototype is created with the following
   -- constants at chunk parsing. After adding this constant to
   -- the function's prototype it will be marked as gray during
-  -- propogate phase.
+  -- propagate phase.
   local function usets() uv = '' end
   _G.usets = usets
 end
diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
index a282a10f..f02bd05f 100644
--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
@@ -20,9 +20,9 @@ local finish = os.clock()
 
 profile.stop()
 
--- XXX: The bug is occured as stopping of callbacks invocation,
+-- XXX: The bug is occurred as stopping of callbacks invocation,
 -- when a new tick strikes inside `gc_call_finalizer()`.
--- The amount of successfull callbacks isn't stable (2-15).
+-- The amount of successful callbacks isn't stable (2-15).
 -- So, assume that amount of profiling samples should be at least
 -- more than 0.5 intervals of time during sampling.
 test:ok(nsamples >= 0.5 * (finish - start) * 1e3 / INTERVAL,
diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
index 791107ba..96ebf92c 100644
--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
@@ -11,7 +11,7 @@ local function do_test()
   local recursive_f
   local function errfunc()
     xpcall(recursive_f, errfunc)
-    -- Since this error is occured on snapshot restoration and can
+    -- Since this error is occurred on snapshot restoration and can
     -- be handled by compiler itself, we shouldn't bother a user
     -- with it.
     handler_is_called = true
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 3cb5c8be..20cfef21 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -52,13 +52,13 @@ local function generate_output(filename, payload)
   collectgarbage()
 
   local res, err = misc.memprof.start(filename)
-  -- Should start succesfully.
+  -- Should start successfully.
   assert(res, err)
 
   payload()
 
   res, err = misc.memprof.stop()
-  -- Should stop succesfully.
+  -- Should stop successfully.
   assert(res, err)
 end
 
@@ -188,7 +188,7 @@ test:test("output", function(subtest)
   -- Check allocation reports. The second argument is a line
   -- number of the allocation event itself. The third is a line
   -- number of the corresponding function definition. The last
-  -- one is the number of allocations. 1 event - alocation of
+  -- one is the number of allocations. 1 event - allocation of
   -- table by itself + 1 allocation of array part as far it is
   -- bigger than LJ_MAX_COLOSIZE (16).
   subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
index 8559ee52..55b7dd72 100644
--- a/test/tarantool-tests/tap.lua
+++ b/test/tarantool-tests/tap.lua
@@ -1,6 +1,6 @@
 --- tap.lua internal file.
 ---
---- The Test Anything Protocol vesion 13 producer.
+--- The Test Anything Protocol version 13 producer.
 ---
 
 -- Initializer FFI for <iscdata> check.
@@ -79,7 +79,7 @@ local function ok(test, cond, message, extra)
   io.write(tindent, ("line:\t%s\n"):format(trace[#trace].line))
   for frameno, frame in ipairs(trace) do
     io.write(tindent, ("frame #%d\n"):format(frameno))
-    -- XXX: Use "half indent" to dump <frame> fiels.
+    -- XXX: Use "half indent" to dump <frame> fields.
     local findent = indent(0.5) .. tindent
     for key, value in pairs(frame) do
       io.write(findent, ("%s:\t%s\n"):format(key, value))
diff --git a/test/tarantool-tests/unit-jit-parse.test.lua b/test/tarantool-tests/unit-jit-parse.test.lua
index 2e6a2228..c0e699c2 100644
--- a/test/tarantool-tests/unit-jit-parse.test.lua
+++ b/test/tarantool-tests/unit-jit-parse.test.lua
@@ -39,7 +39,7 @@ local loop_trace = traces[1]
 for irnum = 1, N_TESTS do
   local ir_pattern = expected_irs[irnum]
   local irref = loop_trace:has_ir(ir_pattern)
-  test:ok(irref, 'find IR refernce by pattern: ' .. ir_pattern)
+  test:ok(irref, 'find IR reference by pattern: ' .. ir_pattern)
 end
 
 test:done(true)
diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
index d865267b..2c09db90 100644
--- a/tools/memprof/parse.lua
+++ b/tools/memprof/parse.lua
@@ -52,14 +52,14 @@ local function link_to_previous(heap_chunk, e, nsize)
     if not e.primary[heap_chunk[2]] then
       e.primary[heap_chunk[2]] = {
         loc = heap_chunk[3],
-        alloced = 0,
+        allocated = 0,
         freed = 0,
         count = 0,
       }
     end
     -- Save information about delta for memory heap.
     local location_data = e.primary[heap_chunk[2]]
-    location_data.alloced = location_data.alloced + nsize
+    location_data.allocated = location_data.allocated + nsize
     location_data.freed = location_data.freed + heap_chunk[1]
     location_data.count = location_data.count + 1
   end
diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
index 0bcb965b..9dc202ae 100644
--- a/tools/memprof/process.lua
+++ b/tools/memprof/process.lua
@@ -39,8 +39,8 @@ function M.form_heap_delta(events, symbols)
       for _, heap_chunk in pairs(event.primary) do
         local ev_line = symtab.demangle(symbols, heap_chunk.loc)
 
-        if (heap_chunk.alloced > 0) then
-          dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
+        if (heap_chunk.allocated > 0) then
+          dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.allocated
           dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.count
         end
 
-- 
2.34.1

[Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

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
target "check", that combines LuaJIT-luacheck and LuaJIT-checkpatch. By
default CMake looking for checkpatch.pl in a directory "checkpatch" in
LuaJIT repository root directory and in a directories specified in PATH.

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()
+  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

[Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Patch adds a GitHub Action, that clones checkpatch repository, and adds
a job, that runs checkpatch [1] using CMake target "LuaJIT-checkpatch"
against commits on top of the master branch.

1. https://github.com/tarantool/checkpatch
---
 .github/actions/checkpatch/action.yml | 11 +++++++++++
 .github/workflows/lint.yml            | 20 ++++++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 .github/actions/checkpatch/action.yml

diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
new file mode 100644
index 00000000..cd2cce62
--- /dev/null
+++ b/.github/actions/checkpatch/action.yml
@@ -0,0 +1,11 @@
+name: Setup checkpatch
+description: Setup a script for checking for codestyle and grammar errors
+runs:
+  using: composite
+  steps:
+    - uses: actions/checkout@v3
+      with:
+        repository: tarantool/checkpatch
+        path: 'checkpatch'
+    - run: apt install -y codespell
+      shell: bash
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
index 8154a622..db3af975 100644
--- a/.github/workflows/lint.yml
+++ b/.github/workflows/lint.yml
@@ -51,3 +51,23 @@ jobs:
       - name: luacheck
         run: cmake --build . --target LuaJIT-luacheck
         working-directory: ${{ env.BUILDDIR }}
+
+  checkpatch:
+    runs-on: [self-hosted, lightweight, Linux, x86_64]
+
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+          submodules: recursive
+      - name: checkpatch
+        uses: ./.github/actions/checkpatch
+      - name: environment
+        uses: ./.github/actions/setup
+      - name: make tarantool/master available
+        run: git checkout tarantool/master && git checkout -
+      - name: configure
+        run: cmake -S . -B ${{ env.BUILDDIR }}
+      - name: checkpatch
+        run: cmake --build . --target LuaJIT-checkpatch
+        working-directory: ${{ env.BUILDDIR }}
-- 
2.34.1

[Tarantool-patches] [PATCH 5/5][v3] test: fix codestyle

[Sergey Bronnikov via Tarantool-patches]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Warnings found by checkpatch.
---
 test/tarantool-c-tests/CMakeLists.txt                        | 1 -
 test/tarantool-c-tests/test.c                                | 2 +-
 test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
index 17255345..4b1d8832 100644
--- a/test/tarantool-c-tests/CMakeLists.txt
+++ b/test/tarantool-c-tests/CMakeLists.txt
@@ -66,4 +66,3 @@ add_custom_command(TARGET tarantool-c-tests
     ${C_TEST_FLAGS}
   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
 )
-
diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
index 74cba3a3..7907c12a 100644
--- a/test/tarantool-c-tests/test.c
+++ b/test/tarantool-c-tests/test.c
@@ -164,7 +164,7 @@ static void test_diagnostic(void)
 	while ((ent_end = strchr(ent, '\n')) != NULL) {
 		char *next_ent = ent_end + 1;
 		/*
-		 * Limit string with with the zero byte for
+		 * Limit string with the zero byte for
 		 * formatted output. Anyway, don't need this \n
 		 * anymore.
 		 */
diff --git a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
index 5159ac32..ee0f5583 100644
--- a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
+++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
@@ -85,7 +85,7 @@ test:ok(not traceinfo(2), 'the second trace should not be compiled')
 jit.on()
 
 for i = 1, N_ITERATIONS do
-  -- Check that that all lookups are correct and there is no
+  -- Check that all lookups are correct and there is no
   -- value from other cdata stored in the table.
   test:ok(result_tab[i] == MAGIC, 'correct hash lookup from the table')
 end
-- 
2.34.1

Re: [Tarantool-patches] [PATCH 1/5][v3] ci: fix a step name

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 969 bytes --]


Hi!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 2 августа 2023, 11:56 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Fixed an incorrect name in a github actions step.
>---
> .github/workflows/lint.yml | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
>index 70c98104..8154a622 100644
>--- a/.github/workflows/lint.yml
>+++ b/.github/workflows/lint.yml
>@@ -48,6 +48,6 @@ jobs:
>           echo CMAKE_BUILD_PARALLEL_LEVEL=$(($(nproc) + 1)) | tee -a $GITHUB_ENV
>       - name: configure
>         run: cmake -S . -B ${{ env.BUILDDIR }} -G Ninja
>- - name: test
>+ - name: luacheck
>         run: cmake --build . --target LuaJIT-luacheck
>         working-directory: ${{ env.BUILDDIR }}
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 1684 bytes --]

Re: [Tarantool-patches] [PATCH 2/5][v3] codehealth: fix typos

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 15435 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 2 августа 2023, 11:56 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Fix typos found with codespell in files with our own source code.
>---
> src/lj_sysprof.c | 2 +-
> src/lj_wbuf.h | 2 +-
> src/luajit-gdb.py | 6 +++---
> src/luajit_lldb.py | 6 +++---
> test/LuaJIT-tests/src/ctest.c | 2 +-
> test/tarantool-tests/CMakeLists.txt | 2 +-
> test/tarantool-tests/fix-emit-rma.test.lua | 2 +-
> test/tarantool-tests/gh-4199-gc64-fuse.test.lua | 2 +-
> .../gh-6096-external-unwinding-on-arm64.test.lua | 2 +-
> test/tarantool-tests/gh-6163-min-max.test.lua | 2 +-
> test/tarantool-tests/lj-416-xor-before-jcc.test.lua | 2 +-
> .../lj-426-arm64-incorrect-check-closed-uv.test.lua | 2 +-
> .../lj-512-profiler-hook-finalizers.test.lua | 4 ++--
> test/tarantool-tests/lj-603-err-snap-restore.test.lua | 2 +-
> test/tarantool-tests/misclib-memprof-lapi.test.lua | 6 +++---
> test/tarantool-tests/tap.lua | 4 ++--
> test/tarantool-tests/unit-jit-parse.test.lua | 2 +-
> tools/memprof/parse.lua | 4 ++--
> tools/memprof/process.lua | 4 ++--
> 19 files changed, 29 insertions(+), 29 deletions(-)
>
>diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c
>index 2e9ed9b3..745eb1a3 100644
>--- a/src/lj_sysprof.c
>+++ b/src/lj_sysprof.c
>@@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L)
>   if (SPS_HALT == sp->state) {
>     errno = sp->saved_errno;
>     sp->state = SPS_IDLE;
>- /* wbuf was terminated when error occured. */
>+ /* wbuf was terminated when error occurred. */
>     return PROFILE_ERRIO;
>   }
> 
>diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h
>index 9eaa5e51..33ec8463 100644
>--- a/src/lj_wbuf.h
>+++ b/src/lj_wbuf.h
>@@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n);
> /* Write string to the buffer. */
> void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s);
> 
>-/* Immediatly flush the buffer. */
>+/* Immediately flush the buffer. */
> void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf);
> 
> /* Check flags. */
>diff --git a/src/luajit-gdb.py b/src/luajit-gdb.py
>index 96ee2289..afdfecd9 100644
>--- a/src/luajit-gdb.py
>+++ b/src/luajit-gdb.py
>@@ -268,7 +268,7 @@ def strdata(obj):
>     try:
>         return str(cast('char *', cast('GCstr *', obj) + 1))[len(PADDING):]
>     except UnicodeEncodeError:
>- return "<luajit-gdb: error occured while rendering non-ascii slot>"
>+ return "<luajit-gdb: error occurred while rendering non-ascii slot>"
> 
> def itypemap(o):
>     if LJ_64 and not LJ_GC64:
>@@ -606,7 +606,7 @@ class LJDumpTable(LJBase):
>     '''
> lj-tab <GCtab *>
> 
>-The command receives a GCtab adress and dumps the table contents:
>+The command receives a GCtab address and dumps the table contents:
> * Metatable address whether the one is set
> * Array part <asize> slots:
>   <aslot ptr>: [<index>]: <tv>
>@@ -677,7 +677,7 @@ coroutine guest stack:
>     + CP: Protected C frame
>     + PP: VM performs a call as a result of executinig pcall or xpcall
> 
>-If L is ommited the main coroutine is used.
>+If L is omitted the main coroutine is used.
>     '''
> 
>     def invoke(self, arg, from_tty):
>diff --git a/src/luajit_lldb.py b/src/luajit_lldb.py
>index 9ee10269..4d7bef81 100644
>--- a/src/luajit_lldb.py
>+++ b/src/luajit_lldb.py
>@@ -490,7 +490,7 @@ def strdata(obj):
>         ptr = cast('char *', obj + 1)
>         return ptr.summary
>     except UnicodeEncodeError:
>- return "<luajit-lldb: error occured while rendering non-ascii slot>"
>+ return "<luajit-lldb: error occurred while rendering non-ascii slot>"
> 
> def itype(o):
>     return tou32(o.it64 >> 47) if LJ_GC64 else o.it
>@@ -909,7 +909,7 @@ class LJDumpTable(Command):
>     '''
> lj-tab <GCtab *>
> 
>-The command receives a GCtab adress and dumps the table contents:
>+The command receives a GCtab address and dumps the table contents:
> * Metatable address whether the one is set
> * Array part <asize> slots:
>   <aslot ptr>: [<index>]: <tv>
>@@ -979,7 +979,7 @@ coroutine guest stack:
>     + CP: Protected C frame
>     + PP: VM performs a call as a result of executinig pcall or xpcall
> 
>-If L is ommited the main coroutine is used.
>+If L is omitted the main coroutine is used.
>     '''
>     def execute(self, debugger, args, result):
>         l = self.parse(args)
>diff --git a/test/LuaJIT-tests/src/ctest.c b/test/LuaJIT-tests/src/ctest.c
>index d257567b..e99f2306 100644
>--- a/test/LuaJIT-tests/src/ctest.c
>+++ b/test/LuaJIT-tests/src/ctest.c
>@@ -234,7 +234,7 @@ static int costatus(lua_State *L, lua_State *co) {
>       else
>  return CO_SUS; /* initial state */
>     }
>- default: /* some error occured */
>+ default: /* some error occurred */
>       return CO_DEAD;
>   }
> }
>diff --git a/test/tarantool-tests/CMakeLists.txt b/test/tarantool-tests/CMakeLists.txt
>index 6218f76a..f0bf8efb 100644
>--- a/test/tarantool-tests/CMakeLists.txt
>+++ b/test/tarantool-tests/CMakeLists.txt
>@@ -132,7 +132,7 @@ else()
> endif()
> 
> # LUA_CPATH and LD_LIBRARY_PATH variables and also TESTLIBS list
>-# with dependecies are set in scope of BuildTestLib macro.
>+# with dependencies are set in scope of BuildTestLib macro.
> add_custom_target(tarantool-tests
>   DEPENDS ${LUAJIT_TEST_BINARY} ${TESTLIBS}
> )
>diff --git a/test/tarantool-tests/fix-emit-rma.test.lua b/test/tarantool-tests/fix-emit-rma.test.lua
>index 42804447..cc97ff4d 100644
>--- a/test/tarantool-tests/fix-emit-rma.test.lua
>+++ b/test/tarantool-tests/fix-emit-rma.test.lua
>@@ -4,7 +4,7 @@ local test = tap.test('fix-emit-rma'):skipcond({
>   ['Test requires GC64 mode enabled'] = not require('ffi').abi('gc64'),
> })
> 
>--- Need to test 2 cases of `emit_rma()` particulary on x64:
>+-- Need to test 2 cases of `emit_rma()` particularly on x64:
> -- * `IR_LDEXP` with `fld` instruction for loading constant
> -- number `TValue` by address.
> -- * `IR_OBAR` with the corresponding `test` instruction on
>diff --git a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>index ca050cf0..877f7884 100644
>--- a/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>+++ b/test/tarantool-tests/gh-4199-gc64-fuse.test.lua
>@@ -44,7 +44,7 @@ for n = 1, 100 do
>       for i = 1, 5 do
>         -- This constant fusion leads to the test failure.
>         a[i] = 0
>- -- This summ is not necessarry but decreases the amount of
>+ -- This summ is not necessary but decreases the amount of
>         -- iterations.
>         a[i] = a[i] + x + y
>         -- Use all FPR registers and one value from the memory
>diff --git a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>index 097e771c..00f58642 100644
>--- a/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>+++ b/test/tarantool-tests/gh-6096-external-unwinding-on-arm64.test.lua
>@@ -1,6 +1,6 @@
> local tap = require('tap')
> 
>--- Test file to check correctnes of external unwinding
>+-- Test file to check correctness of external unwinding
> -- in LuaJIT.
> -- See also  https://github.com/LuaJIT/LuaJIT/issues/698 ,
> --  https://github.com/LuaJIT/LuaJIT/pull/757 .
>diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
>index 63437955..3b7a8e03 100644
>--- a/test/tarantool-tests/gh-6163-min-max.test.lua
>+++ b/test/tarantool-tests/gh-6163-min-max.test.lua
>@@ -53,7 +53,7 @@ local x = 1
> 
> jit.opt.start('hotloop=1')
> 
>--- XXX: Looping over the operations and their arguments breakes the
>+-- XXX: Looping over the operations and their arguments breaks the
> -- semantics of some optimization tests below. The cases are
> -- copy-pasted to preserve optimization semantics.
> 
>diff --git a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>index 605bb92a..f71c9ee4 100644
>--- a/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>+++ b/test/tarantool-tests/lj-416-xor-before-jcc.test.lua
>@@ -41,7 +41,7 @@ local testxor = ffi.load('libtestxor')
> -- pressure and specific registers allocations.
> local handler = setmetatable({}, {
>   __newindex = function ()
>- -- 0 and nil are suggested as differnt constant-zero values
>+ -- 0 and nil are suggested as different constant-zero values
>     -- for the call and occupied different registers.
>     testxor.test_xor_func(0, 0, 0, 0, 0, 0, nil, 0)
>   end
>diff --git a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>index a689ed60..ea01ba06 100644
>--- a/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>+++ b/test/tarantool-tests/lj-426-arm64-incorrect-check-closed-uv.test.lua
>@@ -14,7 +14,7 @@ do
>   -- The function's prototype is created with the following
>   -- constants at chunk parsing. After adding this constant to
>   -- the function's prototype it will be marked as gray during
>- -- propogate phase.
>+ -- propagate phase.
>   local function usets() uv = '' end
>   _G.usets = usets
> end
>diff --git a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>index a282a10f..f02bd05f 100644
>--- a/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>+++ b/test/tarantool-tests/lj-512-profiler-hook-finalizers.test.lua
>@@ -20,9 +20,9 @@ local finish = os.clock()
> 
> profile.stop()
> 
>--- XXX: The bug is occured as stopping of callbacks invocation,
>+-- XXX: The bug is occurred as stopping of callbacks invocation,
> -- when a new tick strikes inside `gc_call_finalizer()`.
>--- The amount of successfull callbacks isn't stable (2-15).
>+-- The amount of successful callbacks isn't stable (2-15).
> -- So, assume that amount of profiling samples should be at least
> -- more than 0.5 intervals of time during sampling.
> test:ok(nsamples >= 0.5 * (finish - start) * 1e3 / INTERVAL,
>diff --git a/test/tarantool-tests/lj-603-err-snap-restore.test.lua b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>index 791107ba..96ebf92c 100644
>--- a/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>+++ b/test/tarantool-tests/lj-603-err-snap-restore.test.lua
>@@ -11,7 +11,7 @@ local function do_test()
>   local recursive_f
>   local function errfunc()
>     xpcall(recursive_f, errfunc)
>- -- Since this error is occured on snapshot restoration and can
>+ -- Since this error is occurred on snapshot restoration and can
>     -- be handled by compiler itself, we shouldn't bother a user
>     -- with it.
>     handler_is_called = true
>diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>index 3cb5c8be..20cfef21 100644
>--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
>+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
>@@ -52,13 +52,13 @@ local function generate_output(filename, payload)
>   collectgarbage()
> 
>   local res, err = misc.memprof.start(filename)
>- -- Should start succesfully.
>+ -- Should start successfully.
>   assert(res, err)
> 
>   payload()
> 
>   res, err = misc.memprof.stop()
>- -- Should stop succesfully.
>+ -- Should stop successfully.
>   assert(res, err)
> end
> 
>@@ -188,7 +188,7 @@ test:test("output", function(subtest)
>   -- Check allocation reports. The second argument is a line
>   -- number of the allocation event itself. The third is a line
>   -- number of the corresponding function definition. The last
>- -- one is the number of allocations. 1 event - alocation of
>+ -- one is the number of allocations. 1 event - allocation of
>   -- table by itself + 1 allocation of array part as far it is
>   -- bigger than LJ_MAX_COLOSIZE (16).
>   subtest:ok(check_alloc_report(alloc, { line = 37, linedefined = 35 }, 2))
>diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
>index 8559ee52..55b7dd72 100644
>--- a/test/tarantool-tests/tap.lua
>+++ b/test/tarantool-tests/tap.lua
>@@ -1,6 +1,6 @@
> --- tap.lua internal file.
> ---
>---- The Test Anything Protocol vesion 13 producer.
>+--- The Test Anything Protocol version 13 producer.
> ---
> 
> -- Initializer FFI for <iscdata> check.
>@@ -79,7 +79,7 @@ local function ok(test, cond, message, extra)
>   io.write(tindent, ("line:\t%s\n"):format(trace[#trace].line))
>   for frameno, frame in ipairs(trace) do
>     io.write(tindent, ("frame #%d\n"):format(frameno))
>- -- XXX: Use "half indent" to dump <frame> fiels.
>+ -- XXX: Use "half indent" to dump <frame> fields.
>     local findent = indent(0.5) .. tindent
>     for key, value in pairs(frame) do
>       io.write(findent, ("%s:\t%s\n"):format(key, value))
>diff --git a/test/tarantool-tests/unit-jit-parse.test.lua b/test/tarantool-tests/unit-jit-parse.test.lua
>index 2e6a2228..c0e699c2 100644
>--- a/test/tarantool-tests/unit-jit-parse.test.lua
>+++ b/test/tarantool-tests/unit-jit-parse.test.lua
>@@ -39,7 +39,7 @@ local loop_trace = traces[1]
> for irnum = 1, N_TESTS do
>   local ir_pattern = expected_irs[irnum]
>   local irref = loop_trace:has_ir(ir_pattern)
>- test:ok(irref, 'find IR refernce by pattern: ' .. ir_pattern)
>+ test:ok(irref, 'find IR reference by pattern: ' .. ir_pattern)
> end
> 
> test:done(true)
>diff --git a/tools/memprof/parse.lua b/tools/memprof/parse.lua
>index d865267b..2c09db90 100644
>--- a/tools/memprof/parse.lua
>+++ b/tools/memprof/parse.lua
>@@ -52,14 +52,14 @@ local function link_to_previous(heap_chunk, e, nsize)
>     if not e.primary[heap_chunk[2]] then
>       e.primary[heap_chunk[2]] = {
>         loc = heap_chunk[3],
>- alloced = 0,
>+ allocated = 0,
>         freed = 0,
>         count = 0,
>       }
>     end
>     -- Save information about delta for memory heap.
>     local location_data = e.primary[heap_chunk[2]]
>- location_data.alloced = location_data.alloced + nsize
>+ location_data.allocated = location_data.allocated + nsize
>     location_data.freed = location_data.freed + heap_chunk[1]
>     location_data.count = location_data.count + 1
>   end
>diff --git a/tools/memprof/process.lua b/tools/memprof/process.lua
>index 0bcb965b..9dc202ae 100644
>--- a/tools/memprof/process.lua
>+++ b/tools/memprof/process.lua
>@@ -39,8 +39,8 @@ function M.form_heap_delta(events, symbols)
>       for _, heap_chunk in pairs(event.primary) do
>         local ev_line = symtab.demangle(symbols, heap_chunk.loc)
> 
>- if (heap_chunk.alloced > 0) then
>- dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.alloced
>+ if (heap_chunk.allocated > 0) then
>+ dheap[ev_line].dbytes = dheap[ev_line].dbytes + heap_chunk.allocated
>           dheap[ev_line].nalloc = dheap[ev_line].nalloc + heap_chunk.count
>         end
> 
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 19064 bytes --]

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3412 bytes --]


Hi, Sergey!
Please consider my comments below.
 
> 
>>From: Sergey Bronnikov < sergeyb@tarantool.org >
>>
>>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/
>>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/
>>LuaJIT repository root directory and in a directories specified in PATH.
>Typo: s/LuaJIT/the LuaJIT/
>Typo: s/a directories/directories/
> 
>>
>>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.
>>+ 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

[-- Attachment #2: Type: text/html, Size: 5110 bytes --]

Re: [Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2158 bytes --]


Hi, Sergey!
Thanks for the patch!
LGTM
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 2 августа 2023, 11:58 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Patch adds a GitHub Action, that clones checkpatch repository, and adds
>a job, that runs checkpatch [1] using CMake target "LuaJIT-checkpatch"
>against commits on top of the master branch.
>
>1.  https://github.com/tarantool/checkpatch
>---
> .github/actions/checkpatch/action.yml | 11 +++++++++++
> .github/workflows/lint.yml | 20 ++++++++++++++++++++
> 2 files changed, 31 insertions(+)
> create mode 100644 .github/actions/checkpatch/action.yml
>
>diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml
>new file mode 100644
>index 00000000..cd2cce62
>--- /dev/null
>+++ b/.github/actions/checkpatch/action.yml
>@@ -0,0 +1,11 @@
>+name: Setup checkpatch
>+description: Setup a script for checking for codestyle and grammar errors
>+runs:
>+ using: composite
>+ steps:
>+ - uses: actions/checkout@v3
>+ with:
>+ repository: tarantool/checkpatch
>+ path: 'checkpatch'
>+ - run: apt install -y codespell
>+ shell: bash
>diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
>index 8154a622..db3af975 100644
>--- a/.github/workflows/lint.yml
>+++ b/.github/workflows/lint.yml
>@@ -51,3 +51,23 @@ jobs:
>       - name: luacheck
>         run: cmake --build . --target LuaJIT-luacheck
>         working-directory: ${{ env.BUILDDIR }}
>+
>+ checkpatch:
>+ runs-on: [self-hosted, lightweight, Linux, x86_64]
>+
>+ steps:
>+ - uses: actions/checkout@v3
>+ with:
>+ fetch-depth: 0
>+ submodules: recursive
>+ - name: checkpatch
>+ uses: ./.github/actions/checkpatch
>+ - name: environment
>+ uses: ./.github/actions/setup
>+ - name: make tarantool/master available
>+ run: git checkout tarantool/master && git checkout -
>+ - name: configure
>+ run: cmake -S . -B ${{ env.BUILDDIR }}
>+ - name: checkpatch
>+ run: cmake --build . --target LuaJIT-checkpatch
>+ working-directory: ${{ env.BUILDDIR }}
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 2956 bytes --]

Re: [Tarantool-patches] [PATCH 5/5][v3] test: fix codestyle

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2220 bytes --]


Hi, Sergey!
LGTM, except for a commit message.
I think it would be better to rephrase it like the following:
| This patch fixes warnings found by the checkpatch.pl script.
--
Best regards,
Maxim Kokryashkin
 
  
>Среда, 2 августа 2023, 11:58 +03:00 от Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org>:
> 
>From: Sergey Bronnikov < sergeyb@tarantool.org >
>
>Warnings found by checkpatch.
>---
> test/tarantool-c-tests/CMakeLists.txt | 1 -
> test/tarantool-c-tests/test.c | 2 +-
> test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua | 2 +-
> 3 files changed, 2 insertions(+), 3 deletions(-)
>
>diff --git a/test/tarantool-c-tests/CMakeLists.txt b/test/tarantool-c-tests/CMakeLists.txt
>index 17255345..4b1d8832 100644
>--- a/test/tarantool-c-tests/CMakeLists.txt
>+++ b/test/tarantool-c-tests/CMakeLists.txt
>@@ -66,4 +66,3 @@ add_custom_command(TARGET tarantool-c-tests
>     ${C_TEST_FLAGS}
>   WORKING_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}
> )
>-
>diff --git a/test/tarantool-c-tests/test.c b/test/tarantool-c-tests/test.c
>index 74cba3a3..7907c12a 100644
>--- a/test/tarantool-c-tests/test.c
>+++ b/test/tarantool-c-tests/test.c
>@@ -164,7 +164,7 @@ static void test_diagnostic(void)
>  while ((ent_end = strchr(ent, '\n')) != NULL) {
>  char *next_ent = ent_end + 1;
>  /*
>- * Limit string with with the zero byte for
>+ * Limit string with the zero byte for
>  * formatted output. Anyway, don't need this \n
>  * anymore.
>  */
>diff --git a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
>index 5159ac32..ee0f5583 100644
>--- a/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
>+++ b/test/tarantool-tests/lj-356-ir-khash-non-string-obj.test.lua
>@@ -85,7 +85,7 @@ test:ok(not traceinfo(2), 'the second trace should not be compiled')
> jit.on()
> 
> for i = 1, N_ITERATIONS do
>- -- Check that that all lookups are correct and there is no
>+ -- Check that all lookups are correct and there is no
>   -- value from other cdata stored in the table.
>   test:ok(result_tab[i] == MAGIC, 'correct hash lookup from the table')
> end
>--
>2.34.1
 

[-- Attachment #2: Type: text/html, Size: 2923 bytes --]

Re: [Tarantool-patches] [PATCH 5/5][v3] test: fix codestyle

[Sergey Bronnikov via Tarantool-patches]

Hi, Max

On 8/3/23 22:45, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> LGTM, except for a commit message.
> I think it would be better to rephrase it like the following:
> | This patch fixes warnings found by the checkpatch.pl script.
>

Thanks, updated and force-pushed.


> Best regards,
> Maxim Kokryashkin
<snipped>

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 9180 bytes --]

Hi, Max

On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote:
> Hi, Sergey!
> Please consider my comments below.
>
>         From: Sergey Bronnikov <sergeyb@tarantool.org
>         </compose?To=sergeyb@tarantool.org>>
>
>         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 <estetus@gmail.com>
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
>

[-- Attachment #2: Type: text/html, Size: 16388 bytes --]

Re: [Tarantool-patches] [PATCH 1/5][v3] ci: fix a step name

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/5][v3] codehealth: fix typos

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
It's LGTM, but I found more spellcheck errors in the code in:

* <src/*> files, that are maintained by us
| ./src/lj_memprof.c:261: resourses ==> resources
| ./src/lj_memprof.c:390: resourses ==> resources

* <test/LuaJIT-tests/> since they are not probably be updated ever in
  future:
| ./test/LuaJIT-tests/README.md:46: alot ==> a lot, allot

* <test/PUC-Rio-Lua-5.1-tests/> since they are not be updated ever in
  future:
| ./test/PUC-Rio-Lua-5.1-tests/api.lua:20: allignment ==> alignment
| ./test/PUC-Rio-Lua-5.1-tests/api.lua:336: acess ==> access
| ./test/PUC-Rio-Lua-5.1-tests/db.lua:564: acessing ==> accessing
| ./test/PUC-Rio-Lua-5.1-tests/gc.lua:311: ressurect ==> resurrect
| ./test/PUC-Rio-Lua-5.1-tests/locals.lua:118: fiels ==> feels, fields, files, phials
| ./test/PUC-Rio-Lua-5.1-tests/math.lua:168: convertions ==> conversions
| ./test/PUC-Rio-Lua-5.1-tests/strings.lua:159: formated ==> formatted

* <test/lua-Harness-tests/*> -- this is debatable, since this part isn't
  maintained by us (we still may apply patches from the upstream).
| ./test/lua-Harness-tests/231-metatable.t:398: heigth ==> height
| ./test/lua-Harness-tests/232-object.t:119: insuficient ==> insufficient
| ./test/lua-Harness-tests/232-object.t:133: insuficient ==> insufficient
| ./test/lua-Harness-tests/232-object.t:93: classe ==> class, classes
| ./test/lua-Harness-tests/241-standalone.t:241: runned ==> ran, run, ruined
| ./test/lua-Harness-tests/242-luac.t:275: endianess ==> endianness

* <test/tarantool-tests/*>:
| ./test/tarantool-tests/lj-350-sload-typecheck.test.lua:2: asembling ==> assembling
| ./test/tarantool-tests/lj-357-arm64-hrefk.test.lua:20: invarinat ==> invariant
| ./test/tarantool-tests/misclib-memprof-lapi.test.lua:2: indepentent ==> independent

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!

It's really amazing changes: I've already found some typos in my
commit messages and comments!:)

Please, consider my comments below.

On 04.08.23, Sergey Bronnikov via Tarantool-patches wrote:
> Hi, Max
> 
> On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote:
> > Hi, Sergey!
> > Please consider my comments below.

<snipped>

> >
> >         Patch introduces two new CMake targets: "LuaJIT-checkpatch",

Typo: s/Patch/The patch/

<snipped>

> >
> >     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.

<snipped>

> 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")
> +

I suppose this should be moved to <cmake/CheckPatch.cmake> too.

>   include(LuaJITUtils)
>   include(SetBuildParallelLevel)
>   include(SetVersion)
> +include(CheckPatch)

And be included from test/CMakeLists.txt, not from root CMakeLists.txt.

> 
>   # --- 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

Typo: s/in commit message/in the commit message/
Typo: s/ and/, and/

> +        # is annoying.

But we always have at least two lines, don't we?

> +        --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

I suppose we should add `--ignore PREFER_FALLTHROUGH`, too.
| ERROR:PREFER_FALLTHROUGH: Prefer 'FALLTHROUGH;' over fallthrough comment

Since it's already used in the codebase:
| $ grep fallthrough -R . | wc -l
| 47

> +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}

Also, we can't just check all patches with this since it will have
TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and
include back files that are maintained by us.

Without this, it becomes pointless, since we either
* will have red always checkpatch job and don't be aware, so can miss
  some errors, either
* will set ignore all (or something like it) to ignore all checkpatch
  warnings, including meaningful one.

> +  )
> +else()
> +  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch 
> target is dummy")

Please, split this line into several.

Minor: I suggest to rename it to the WARN_MSG.
Feel free to ignore.

> +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
> +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
> +    COMMENT ${MSG}
> +  )
> +endif()

<snipped>

> >

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 5/5][v3] test: fix codestyle

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM!

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the patch!
LGTM, except a single nit regarding the commit message.

On 02.08.23, Sergey Bronnikov wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Patch adds a GitHub Action, that clones checkpatch repository, and adds

Typo: s/Patch/The patch/

> a job, that runs checkpatch [1] using CMake target "LuaJIT-checkpatch"
> against commits on top of the master branch.
> 

<snipped>

> 

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 2/5][v3] codehealth: fix typos

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey!


thanks for review

On 8/9/23 15:58, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> It's LGTM, but I found more spellcheck errors in the code in:
>
> * <src/*> files, that are maintained by us
> | ./src/lj_memprof.c:261: resourses ==> resources
> | ./src/lj_memprof.c:390: resourses ==> resources
>
> * <test/LuaJIT-tests/> since they are not probably be updated ever in
>    future:
> | ./test/LuaJIT-tests/README.md:46: alot ==> a lot, allot
>
> * <test/PUC-Rio-Lua-5.1-tests/> since they are not be updated ever in
>    future:
> | ./test/PUC-Rio-Lua-5.1-tests/api.lua:20: allignment ==> alignment
> | ./test/PUC-Rio-Lua-5.1-tests/api.lua:336: acess ==> access
> | ./test/PUC-Rio-Lua-5.1-tests/db.lua:564: acessing ==> accessing
> | ./test/PUC-Rio-Lua-5.1-tests/gc.lua:311: ressurect ==> resurrect
> | ./test/PUC-Rio-Lua-5.1-tests/locals.lua:118: fiels ==> feels, fields, files, phials
> | ./test/PUC-Rio-Lua-5.1-tests/math.lua:168: convertions ==> conversions
> | ./test/PUC-Rio-Lua-5.1-tests/strings.lua:159: formated ==> formatted
>
> * <test/lua-Harness-tests/*> -- this is debatable, since this part isn't
>    maintained by us (we still may apply patches from the upstream).
> | ./test/lua-Harness-tests/231-metatable.t:398: heigth ==> height
> | ./test/lua-Harness-tests/232-object.t:119: insuficient ==> insufficient
> | ./test/lua-Harness-tests/232-object.t:133: insuficient ==> insufficient
> | ./test/lua-Harness-tests/232-object.t:93: classe ==> class, classes
> | ./test/lua-Harness-tests/241-standalone.t:241: runned ==> ran, run, ruined
> | ./test/lua-Harness-tests/242-luac.t:275: endianess ==> endianness
>
> * <test/tarantool-tests/*>:
> | ./test/tarantool-tests/lj-350-sload-typecheck.test.lua:2: asembling ==> assembling
> | ./test/tarantool-tests/lj-357-arm64-hrefk.test.lua:20: invarinat ==> invariant
> | ./test/tarantool-tests/misclib-memprof-lapi.test.lua:2: indepentent ==> independent
>
> <snipped>
>
All fixes applied except typos for lua-Harness. I sent a patch with 
fixes to maintainer of lua-Harness François Perrad.

See the patch below.



diff --git a/src/lj_memprof.c b/src/lj_memprof.c
index c600c4f0..2e33d88d 100644
--- a/src/lj_memprof.c
+++ b/src/lj_memprof.c
@@ -258,7 +258,7 @@ int lj_memprof_start(struct lua_State *L, const 
struct lj_memprof_options *opt)
    lua_assert(opt->len != 0);

    if (mp->state != MPS_IDLE) {
-    /* Clean up resourses. Ignore possible errors. */
+    /* Clean up resources. Ignore possible errors. */
      opt->on_stop(opt->ctx, opt->buf);
      return PROFILE_ERRRUN;
    }
@@ -387,7 +387,7 @@ void lj_memprof_add_trace(const struct GCtrace *tr)
  int lj_memprof_start(struct lua_State *L, const struct 
lj_memprof_options *opt)
  {
    UNUSED(L);
-  /* Clean up resourses. Ignore possible errors. */
+  /* Clean up resources. Ignore possible errors. */
    opt->on_stop(opt->ctx, opt->buf);
    return PROFILE_ERRUSE;
  }
diff --git a/test/PUC-Rio-Lua-5.1-tests/api.lua 
b/test/PUC-Rio-Lua-5.1-tests/api.lua
index c955ebf9..7ec2f1ba 100644
--- a/test/PUC-Rio-Lua-5.1-tests/api.lua
+++ b/test/PUC-Rio-Lua-5.1-tests/api.lua
@@ -17,7 +17,7 @@ function pack(...) return arg end

  print('testing C API')

--- testing allignment
+-- testing alignment
  a = T.d2s(12458954321123)
  assert(string.len(a) == 8)   -- sizeof(double)
  assert(T.s2d(a) == 12458954321123)
@@ -333,7 +333,7 @@ F = function (x)
    if A ~= nil then
      assert(type(A) == "userdata")
      assert(T.udataval(A) == B)
-    debug.getmetatable(A)    -- just acess it
+    debug.getmetatable(A)    -- just access it
    end
    A = x   -- ressucita userdata
    B = udval
diff --git a/test/PUC-Rio-Lua-5.1-tests/db.lua 
b/test/PUC-Rio-Lua-5.1-tests/db.lua
index b148c2dd..0d4692e1 100644
--- a/test/PUC-Rio-Lua-5.1-tests/db.lua
+++ b/test/PUC-Rio-Lua-5.1-tests/db.lua
@@ -561,7 +561,7 @@ t[1] = "'error'"
  checktraceback(co, t)


--- test acessing line numbers of a coroutine from a resume inside
+-- test accessing line numbers of a coroutine from a resume inside
  -- a C function (this is a known bug in Lua 5.0)

  local function g(x)
diff --git a/test/PUC-Rio-Lua-5.1-tests/gc.lua 
b/test/PUC-Rio-Lua-5.1-tests/gc.lua
index 10e1f2dd..a1c6e7c7 100644
--- a/test/PUC-Rio-Lua-5.1-tests/gc.lua
+++ b/test/PUC-Rio-Lua-5.1-tests/gc.lua
@@ -308,7 +308,7 @@ do
      assert(getmetatable(o) == tt)
      -- create new objects during GC
      local a = 'xuxu'..(10+3)..'joao', {}
-    ___Glob = o  -- ressurect object!
+    ___Glob = o  -- resurrect object!
      newproxy(o)  -- creates a new one with same metatable
      print(">>> closing state " .. "<<<\n")
    end
diff --git a/test/PUC-Rio-Lua-5.1-tests/math.lua 
b/test/PUC-Rio-Lua-5.1-tests/math.lua
index f66ce196..63efc5dd 100644
--- a/test/PUC-Rio-Lua-5.1-tests/math.lua
+++ b/test/PUC-Rio-Lua-5.1-tests/math.lua
@@ -165,7 +165,7 @@ stat(a)

  a = nil

--- testing implicit convertions
+-- testing implicit conversions

  local a,b = '10', '20'
  assert(a*b == 200 and a+b == 30 and a-b == -10 and a/b == 0.5 and -b 
== -20)
diff --git a/test/PUC-Rio-Lua-5.1-tests/strings.lua 
b/test/PUC-Rio-Lua-5.1-tests/strings.lua
index 3cc1c1b2..0818e390 100644
--- a/test/PUC-Rio-Lua-5.1-tests/strings.lua
+++ b/test/PUC-Rio-Lua-5.1-tests/strings.lua
@@ -156,7 +156,7 @@ assert(string.format('"-%20s.20s"', string.rep("%", 
2000)) ==
         string.format("%q", "-"..string.rep("%", 2000)..".20s"))


--- longest number that can be formated
+-- longest number that can be formatted
  assert(string.len(string.format('%99.99f', -1e308)) >= 100)

  assert(loadstring("return 1\n--comentário sem EOL no final")() == 1)
diff --git a/test/tarantool-tests/lj-350-sload-typecheck.test.lua 
b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
index 33380170..c6876473 100644
--- a/test/tarantool-tests/lj-350-sload-typecheck.test.lua
+++ b/test/tarantool-tests/lj-350-sload-typecheck.test.lua
@@ -1,5 +1,5 @@
  local tap = require('tap')
--- Test file to demonstrate the incorrect GC64 JIT asembling
+-- Test file to demonstrate the incorrect GC64 JIT assembling
  -- `IR_SLOAD`.
  -- See also https://github.com/LuaJIT/LuaJIT/pull/350.
  local test = tap.test('lj-350-sload-typecheck'):skipcond({
diff --git a/test/tarantool-tests/lj-357-arm64-hrefk.test.lua 
b/test/tarantool-tests/lj-357-arm64-hrefk.test.lua
index 8db8bbfc..23e06d8c 100644
--- a/test/tarantool-tests/lj-357-arm64-hrefk.test.lua
+++ b/test/tarantool-tests/lj-357-arm64-hrefk.test.lua
@@ -17,7 +17,7 @@ local t = {hrefk = 0}
  -- chooses the same register as a base register for offset and
  -- destination in LDR instruction.
  -- We need 1028 iterations = 1024 iteration for 10 table rehashing
--- (create side traces for invarinat and variant loop part) +
+-- (create side traces for invariant and variant loop part) +
  -- 3 for trace initialize + 1 to run the bad trace.
  local START = 1028
  local STOP = 1
diff --git a/test/tarantool-tests/misclib-memprof-lapi.test.lua 
b/test/tarantool-tests/misclib-memprof-lapi.test.lua
index 20cfef21..8132cf08 100644
--- a/test/tarantool-tests/misclib-memprof-lapi.test.lua
+++ b/test/tarantool-tests/misclib-memprof-lapi.test.lua
@@ -1,5 +1,5 @@
  -- XXX: This comment is a reminder to reimplement memprof tests
--- assertions to make them more indepentent to the changes made.
+-- assertions to make them more independent to the changes made.
  local tap = require("tap")
  local test = tap.test("misc-memprof-lapi"):skipcond({
    ['Test requires JIT enabled'] = not jit.status(),

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey!

On 8/9/23 17:04, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
>
> It's really amazing changes: I've already found some typos in my
> commit messages and comments!:)
>
> Please, consider my comments below.
>
> On 04.08.23, Sergey Bronnikov via Tarantool-patches wrote:
>> Hi, Max
>>
>> On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote:
>>> Hi, Sergey!
>>> Please consider my comments below.
> <snipped>
>
>>>          Patch introduces two new CMake targets: "LuaJIT-checkpatch",
> Typo: s/Patch/The patch/

Fixed.


>
> <snipped>
>
>>>      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.
> <snipped>
>
>> 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")
>> +
> I suppose this should be moved to <cmake/CheckPatch.cmake> too.
No, this var is required for gcovr too and will be used by it in patches 
with coverage support.
>
>>    include(LuaJITUtils)
>>    include(SetBuildParallelLevel)
>>    include(SetVersion)
>> +include(CheckPatch)
> And be included from test/CMakeLists.txt, not from root CMakeLists.txt.
Why? How checkpatch is related to testing?
>
>>    # --- 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
> Typo: s/in commit message/in the commit message/
> Typo: s/ and/, and/

Fixed.


>> +        # is annoying.
> But we always have at least two lines, don't we?
Sometimes no. See commit "codehealth: fix typos" in patch series.
>
>> +        --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
> I suppose we should add `--ignore PREFER_FALLTHROUGH`, too.
> | ERROR:PREFER_FALLTHROUGH: Prefer 'FALLTHROUGH;' over fallthrough comment
>
> Since it's already used in the codebase:
> | $ grep fallthrough -R . | wc -l
> | 47


Added.

>
>> +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
> Also, we can't just check all patches with this since it will have
> TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and
> include back files that are maintained by us.
>
> Without this, it becomes pointless, since we either
> * will have red always checkpatch job and don't be aware, so can miss
>    some errors, either
> * will set ignore all (or something like it) to ignore all checkpatch
>    warnings, including meaningful one.

Agree, but this requires patching checkpatch itself, right now it is not 
possible

to suppress warnings for a certain files in patch. I'll try to implement 
such patch.


>> +  )
>> +else()
>> +  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch
>> target is dummy")
> Please, split this line into several.
>
> Minor: I suggest to rename it to the WARN_MSG.
> Feel free to ignore.

Updated.


>> +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
>> +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
>> +    COMMENT ${MSG}
>> +  )
>> +endif()
> <snipped>
>

Re: [Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

Hi, Sergey

On 8/9/23 17:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except a single nit regarding the commit message.
>
> On 02.08.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Patch adds a GitHub Action, that clones checkpatch repository, and adds
> Typo: s/Patch/The patch/

Fixed.


>> a job, that runs checkpatch [1] using CMake target "LuaJIT-checkpatch"
>> against commits on top of the master branch.
>>
> <snipped>
>

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Sergey Kaplun via Tarantool-patches]

Hi, Sergey!
Thanks for the fixes!

On 09.08.23, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> On 8/9/23 17:04, Sergey Kaplun wrote:

<snipped>

> >>    include(LuaJITUtils)
> >>    include(SetBuildParallelLevel)
> >>    include(SetVersion)
> >> +include(CheckPatch)
> > And be included from test/CMakeLists.txt, not from root CMakeLists.txt.
> Why? How checkpatch is related to testing?

Not so much, but we still have `LuaJIT-luacheck` target in testing. I
expected the same from `LuaJIT-checkpatch` target (and `test` target
depends on `LuaJIT-checkpatch` target).

But we may also move `LuaJIT-luacheck` in the separate include file in the
separate patch series.

> >

<snipped>

> >> +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
> > Also, we can't just check all patches with this since it will have
> > TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and
> > include back files that are maintained by us.
> >
> > Without this, it becomes pointless, since we either
> > * will have red always checkpatch job and don't be aware, so can miss
> >    some errors, either
> > * will set ignore all (or something like it) to ignore all checkpatch
> >    warnings, including meaningful one.
> 
> Agree, but this requires patching checkpatch itself, right now it is not 
> possible
> 
> to suppress warnings for a certain files in patch. I'll try to implement 
> such patch.

Looking forward to it!

> 

<snipped>

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch

[Sergey Bronnikov via Tarantool-patches]

Hi, everyone


Patches rebased to master (tarantool/master) after merging patches with 
flake8

and force-pushed to the branch.


Sergey


On 8/9/23 17:40, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except a single nit regarding the commit message.
>
> On 02.08.23, Sergey Bronnikov wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Patch adds a GitHub Action, that clones checkpatch repository, and adds
> Typo: s/Patch/The patch/
>
>> a job, that runs checkpatch [1] using CMake target "LuaJIT-checkpatch"
>> against commits on top of the master branch.
>>
> <snipped>
>

Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets

[Maxim Kokryashkin via Tarantool-patches]

Hi, Sergey!
LGTM, but I am really concerned about that `src/*` supression problem. 
Looking forward to see the patch with supression in this series.

On Wed, Aug 09, 2023 at 05:55:02PM +0300, Sergey Bronnikov wrote:
> Hi, Sergey!
> 
> On 8/9/23 17:04, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > 
> > It's really amazing changes: I've already found some typos in my
> > commit messages and comments!:)
> > 
> > Please, consider my comments below.
> > 
> > On 04.08.23, Sergey Bronnikov via Tarantool-patches wrote:
> > > Hi, Max
> > > 
> > > On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote:
> > > > Hi, Sergey!
> > > > Please consider my comments below.
> > <snipped>
> > 
> > > >          Patch introduces two new CMake targets: "LuaJIT-checkpatch",
> > Typo: s/Patch/The patch/
> 
> Fixed.
> 
> 
> > 
> > <snipped>
> > 
> > > >      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.
> > <snipped>
> > 
> > > 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")
> > > +
> > I suppose this should be moved to <cmake/CheckPatch.cmake> too.
> No, this var is required for gcovr too and will be used by it in patches
> with coverage support.
> > 
> > >    include(LuaJITUtils)
> > >    include(SetBuildParallelLevel)
> > >    include(SetVersion)
> > > +include(CheckPatch)
> > And be included from test/CMakeLists.txt, not from root CMakeLists.txt.
> Why? How checkpatch is related to testing?
Agree with Sergey Bronnikov here.
> > 
> > >    # --- 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
> > Typo: s/in commit message/in the commit message/
> > Typo: s/ and/, and/
> 
> Fixed.
> 
> 
> > > +        # is annoying.
> > But we always have at least two lines, don't we?
> Sometimes no. See commit "codehealth: fix typos" in patch series.
> > 
> > > +        --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
> > I suppose we should add `--ignore PREFER_FALLTHROUGH`, too.
> > | ERROR:PREFER_FALLTHROUGH: Prefer 'FALLTHROUGH;' over fallthrough comment
> > 
> > Since it's already used in the codebase:
> > | $ grep fallthrough -R . | wc -l
> > | 47
> 
> 
> Added.
> 
> > 
> > > +    WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}
> > Also, we can't just check all patches with this since it will have
> > TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and
> > include back files that are maintained by us.
> > 
> > Without this, it becomes pointless, since we either
> > * will have red always checkpatch job and don't be aware, so can miss
> >    some errors, either
> > * will set ignore all (or something like it) to ignore all checkpatch
> >    warnings, including meaningful one.
> 
> Agree, but this requires patching checkpatch itself, right now it is not
> possible
> 
> to suppress warnings for a certain files in patch. I'll try to implement
> such patch.
> 
> 
> > > +  )
> > > +else()
> > > +  set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch
> > > target is dummy")
> > Please, split this line into several.
> > 
> > Minor: I suggest to rename it to the WARN_MSG.
> > Feel free to ignore.
> 
> Updated.
> 
> 
> > > +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch
> > > +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG}
> > > +    COMMENT ${MSG}
> > > +  )
> > > +endif()
> > <snipped>
Best regards, Maxim Kokryashkin
> >