[Tarantool-patches] [PATCH 5/6] Fix luacheck warnings in src/lua/
Sergey Bronnikov
sergeyb at tarantool.org
Thu Jun 11 15:00:49 MSK 2020
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 at tarantool.org wrote:
> > From: Sergey Bronnikov <sergeyb at tarantool.org>
> >
> > Part of #4681
> >
> > Reviewed-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Reviewed-by: Igor Munkin <imun at tarantool.org>
> >
> > Co-authored-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> > Co-authored-by: Igor Munkin <imun at tarantool.org>
> > ---
> > .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 <self>.
> > - "411", -- Redefining a local variable.
> > - "431", -- Shadowing an upvalue.
> > + "143/debug", -- Accessing an undefined field of a global variable <debug>.
>
> 1. Why are global variables ignored sometimes using '143/<name>', 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 <string>.
> > + "143/table", -- Accessing an undefined field of a global variable <table>.
> > + "212/self", -- Unused argument <self>.
>
> 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@
More information about the Tarantool-patches
mailing list