From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 23181469710 for ; Sat, 6 Jun 2020 21:03:10 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <1d6128a1-127e-8f27-b937-c88219b3f53f@tarantool.org> Date: Sat, 6 Jun 2020 20:03:07 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergeyb@tarantool.org, tarantool-patches@dev.tarantool.org, imun@tarantool.org Cc: alexander.turenko@tarantool.org 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'. > + "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. > + "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. > +}> +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. > + }, > +} > +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. 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. > + }, > +} > 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. > 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. > 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. > 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. > 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? > 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. > if data == nil then > if not errno_is_transient[self._errno] then > return nil, socket_error(self)