From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 5B3AC40F3AE for ; Thu, 11 Jun 2020 15:02:04 +0300 (MSK) Date: Thu, 11 Jun 2020 15:00:49 +0300 From: Sergey Bronnikov Message-ID: <20200611120049.GA66555@pony.bronevichok.ru> References: <1d6128a1-127e-8f27-b937-c88219b3f53f@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1d6128a1-127e-8f27-b937-c88219b3f53f@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: alexander.turenko@tarantool.org, tarantool-patches@dev.tarantool.org Hi, Vlad thanks for review! On 20:03 Sat 06 Jun , Vladislav Shpilevoy wrote: > Thanks for the patch! > > See 11 comments below. > > On 04/06/2020 10:39, sergeyb@tarantool.org wrote: > > From: Sergey Bronnikov > > > > Part of #4681 > > > > Reviewed-by: Vladislav Shpilevoy > > Reviewed-by: Igor Munkin > > > > Co-authored-by: Vladislav Shpilevoy > > Co-authored-by: Igor Munkin > > --- > > .luacheckrc | 28 ++++++++++++++++++++++++---- > > src/lua/argparse.lua | 3 ++- > > src/lua/buffer.lua | 4 ++-- > > src/lua/clock.lua | 2 +- > > src/lua/crypto.lua | 4 ++-- > > src/lua/csv.lua | 5 ++--- > > src/lua/digest.lua | 2 +- > > src/lua/env.lua | 2 +- > > src/lua/fiber.lua | 4 ++-- > > src/lua/fio.lua | 30 ++++++++++++++---------------- > > src/lua/help.lua | 7 ++----- > > src/lua/httpc.lua | 3 --- > > src/lua/init.lua | 4 ++-- > > src/lua/msgpackffi.lua | 22 +++++++++------------- > > src/lua/socket.lua | 16 ++++++++-------- > > src/lua/string.lua | 1 - > > src/lua/swim.lua | 2 +- > > src/lua/tap.lua | 4 ---- > > src/lua/trigger.lua | 3 --- > > 19 files changed, 73 insertions(+), 73 deletions(-) > > > > diff --git a/.luacheckrc b/.luacheckrc > > index b917eb927..fb8b9dfb3 100644 > > --- a/.luacheckrc > > +++ b/.luacheckrc > > @@ -1,9 +1,14 @@ > > std = "luajit" > > globals = {"box", "_TARANTOOL"} > > ignore = { > > - "212/self", -- Unused argument . > > - "411", -- Redefining a local variable. > > - "431", -- Shadowing an upvalue. > > + "143/debug", -- Accessing an undefined field of a global variable . > > 1. Why are global variables ignored sometimes using '143/', sometimes > via adding to the global 'globals', sometimes via the per-file 'globals'? > 'debug', 'string', and 'table' are globals, available in all the sources, > just like 'box'. the main idea was to supress rarely used variables inline and often used variables globally in .luacheckrc. Suggested by Igor in at least [1] and I'm agree with his approach. 1. https://lists.tarantool.org/pipermail/tarantool-patches/2020-May/017108.html > > > + "143/string", -- Accessing an undefined field of a global variable . > > + "143/table", -- Accessing an undefined field of a global variable . > > + "212/self", -- Unused argument . > > 2. This is one of the main reasons, why non-standardized alignment applied > to everything is evil. Diff split into patches does not make much sense, when a > next patch anyway should rewrite 100% of the previous one just to make alignment > correct. I would propose to make the alignment correct right away, from the first > patch. Or remove it. Or make it less strict, so not all lines are required to be > equaly aligned. Whatever helps not to change already fine lines. Moved comments before code. > > + "411", -- Redefining a local variable. > > + "421", -- Shadowing a local variable. > > + "431", -- Shadowing an upvalue. > > + "432", -- Shadowing an upvalue argument. > > } > > @@ -27,3 +32,18 @@ files["extra/dist/tarantoolctl.in"] = { > > "122", -- https://github.com/tarantool/tarantool/issues/4929 > > }, > > } > > +files["src/lua/help.lua"] = { > > + globals = {"help", "tutorial"}, -- globals defined for interactive mode. > > 3. Lets use single style for all comments: start sentences from > capital letters. Instead of switching between lowcase and normal > writing from time to time. Fixed. > > > +}> +files["src/lua/init.lua"] = { > > + globals = {"dostring"}, -- miscellaneous global function definition. > > + ignore = { > > + "122/os", -- set tarantool specific behaviour for os.exit. > > + "142/package", -- add custom functions into Lua package namespace. > > 4. os and package changes can be done using rawset() function. It does not > produce a warning, and behaves in the above cases the same. However this > should be consulted with other reviewers. > > Another option - add 'os' and 'package' to the list of globals. It also fixes > the warnings. Fixed using rawset(). > > > + }, > > +} > > +files["src/lua/swim.lua"] = { > > + ignore = { > > + "212/m", -- respect swim module code style. > > 5. Once again I wonder why do we ignore unused arguments sometimes globaly in > 'ignore = {...}', sometimes per-file like here, sometimes using inlined luacheck > comments. Answered in comment 1. > In this particular case there is even a fourth option - rename 'm' to 'self' in > the single problematic line. Like you did for many similar places. replaced 'm' to 'self'. > > + }, > > +} > > diff --git a/src/lua/argparse.lua b/src/lua/argparse.lua > > index faa0ae130..6c8f10fc1 100644 > > --- a/src/lua/argparse.lua > > +++ b/src/lua/argparse.lua > > @@ -91,7 +91,8 @@ local function convert_parameter(name, convert_from, convert_to) > > end > > > > local function parameters_parse(t_in, options) > > - local t_out, t_in = {}, t_in or {} > > + local t_out = {} > > + t_in = t_in or {} > > 6. Unnecessary diff. At least when I check on top of the branch. change is required, see [1] 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153 > > > diff --git a/src/lua/init.lua b/src/lua/init.lua > > index ff3e74c3c..aea7a7491 100644 > > --- a/src/lua/init.lua > > +++ b/src/lua/init.lua > > @@ -144,9 +144,9 @@ local function gen_search_func(path_fn, templates, need_traverse) > > > > local searchpaths = {} > > > > - for _, path in ipairs(paths) do > > + for _, p in ipairs(paths) do > > for _, template in pairs(templates) do > > - table.insert(searchpaths, fio.pathjoin(path, template)) > > + table.insert(searchpaths, fio.pathjoin(p, template)) > > end > > 7. Ditto. change reverted > > > end > > > > @@ -603,7 +599,7 @@ local function check_offset(offset, len) > > if offset == nil then > > return 1 > > end > > - local offset = ffi.cast('ptrdiff_t', offset) > > + offset = ffi.cast('ptrdiff_t', offset) > > 8. Ditto. change is required, see [1] 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153 > > > if offset < 1 or offset > len then > > error(string.format("offset = %d is out of bounds [1..%d]", > > tonumber(offset), len)) > > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > > index bbdef01e3..2a2c7a32c 100644 > > --- a/src/lua/socket.lua > > +++ b/src/lua/socket.lua > > @@ -1044,7 +1044,7 @@ local function tcp_connect(host, port, timeout) > > boxerrno(0) > > return s > > end > > - local timeout = timeout or TIMEOUT_INFINITY > > + timeout = timeout or TIMEOUT_INFINITY > > 9. Ditto. change is required, see [1] src/lua/socket.lua:344:11: variable timeout was previously defined as an argument on line 340 1. https://gitlab.com/tarantool/tarantool/-/jobs/589857153 > > local stop = fiber.clock() + timeout > > local dns = getaddrinfo(host, port, timeout, { type = 'SOCK_STREAM', > > protocol = 'tcp' }) > > @@ -1319,7 +1319,7 @@ local function lsocket_tcp_getpeername(self) > > return peer.host, tostring(peer.port), peer.family:match("AF_(.*)"):lower() > > end > > > > -local function lsocket_tcp_settimeout(self, value, mode) > > +local function lsocket_tcp_settimeout(self, value, mode) -- luacheck: no unused args > > 10. Mode is really ignored, and this is not some kind of built-in > function with 'fixed' signature. And it is not used 'virtually', > when there is a variable keeping pointer at one function among > many having the same signature. So what is a purpose of keeping it? removed mode arg > > check_socket(self) > > self.timeout = value > > -- mode is effectively ignored > > @@ -1475,7 +1475,7 @@ local function lsocket_tcp_receive(self, pattern, prefix) > > local result = { prefix } > > local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY) > > repeat > > - local data = read(self, LIMIT_INFINITY, timeout, check_infinity) > > + data = read(self, LIMIT_INFINITY, timeout, check_infinity) > > 11. Unnecessary diff. change reverted > > if data == nil then > > if not errno_is_transient[self._errno] then > > return nil, socket_error(self) -- sergeyb@