[Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader.
Sergey Kaplun
skaplun at tarantool.org
Mon Sep 11 11:45:46 MSK 2023
Hi, Sergey!
Thanks for the fixes!
On 07.09.23, Sergey Bronnikov wrote:
> Hi, Sergey
>
>
> On 9/5/23 17:10, Sergey Kaplun wrote:
> > Hi, Sergey!
> > Thanks for the patch!
> > Please, consider my comments below.
> >
> > On 31.08.23, Sergey Bronnikov wrote:
> >> From: Sergey Bronnikov <sergeyb at tarantool.org>
> >>
<snipped>
> >
> >> +end
> >> +
> >> +local function get_file_name(file)
> >> + return file:match("[^/]*$")
> > Minor: it may match the empty string for a directory occasionally:
> > | src/luajit -e 'print([["]]..("/tmp/"):match("[^/]*$")..[["]])'
>
>
> Fixed.
>
> > | ""
> >
> > Nit: use single quotes instead of double quotes if possible.
>
> Without context it is difficult to get what is line you talk about.
>
> As I see everything is fine with quotes in version on the branch.
>
I'm talking about the same line (got this version from the branch):
| +local function basename(path)
| + return path:match("[^/]*$")
| +end
>
> >
> > Nit: `[^/\\]` is better since it also covers Windows.
> > See <test/lua-Harness-tests/314-regex.t:167>
> > | local dirname = arg[0]:gsub('([^/\\]+)$', '')
> > Since we don't support Windows feel free to ignore.
> >
> >> +end
> >> +
> >> +local stdout_msg = 'Lango team'
> >> +local lua_code = ('print(%q)'):format(stdout_msg)
> >> +local fpath = os.tmpname()
> >> +local path_lua = ('%s.lua'):format(fpath)
> >> +local path_c = ('%s.c'):format(fpath)
> >> +local path_so = ('%s.so'):format(fpath)
> > Minor: I suppose it should be renamed to `path_shared`, since on macOS
> > they have the ".dyld" suffix for shared libs. Hence, we need to use the
> > suffix in format of the shared library name too. You may take some
> > inspiration from here [1].
> Fixed.
Hmm, I don't see these updates on the branch [1]. Maybe you forgot to push
your local changes to the repository (*)?
> >> +
<snipped>
> >> +local basedir = function(path)
> >> + local sep = '/'
> > Why do we need an additional variable here?
>
> For clarity.
OK.
>
>
> >
> > Nit: Indent is 4 spaces instead of 2.
> >
> >> + return path:match('(.*' .. sep .. ')') or './'
> > It's better to mention that the pattern matching is greedy, so we match
> > until the last separator.
>
>
> Updated.
This is missing too (*).
>
> >> +end
> >> +
> >> +-- Create a C file with LuaJIT bytecode.
> >> +-- We cannot use utils.makecmd, because command-line generated
> >> +-- by `makecmd` contains `-e` that is incompatible with option `-b`.
> > Nit: comment line width is more than 66 symbols
>
> Fixed.
>
>
> >> +local function create_c_file(pathlua, pathc)
> >> + local lua_path = os.getenv('LUA_PATH')
> >> + local lua_bin = require('utils').exec.luacmd(arg):match('%S+')
> >> + local cmd_fmt = 'LUA_PATH="%s" %s -b %s %s'
> >> + local cmd = (cmd_fmt):format(lua_path, lua_bin, pathlua, pathc)
> >> + local ret = os.execute(cmd)
> >> + assert(ret == 0, 'create a C file with bytecode')
> >> +end
> >> +
> >> +create_c_file(path_lua, path_c)
> >> +assert(file_exists(path_c))
> > Minor: The test flow is a little bit hard to read due to function
> > declarations. Maybe it is better to declare all utility functions first
> > and then use them one by one? This makes control flow easier to read.
> Rearranged, take a look please.
Yes, this is muchc clearer, thanks!
> >
> >> +
<snipped>
> >
> >> + -- It is required to cleanup LUA_PATH, otherwise
> >> + -- LuaJIT loads Lua module, see tarantool-tests/utils/init.lua.
> > Nit: comment line width is more than 66 symbols
> Fixed too.
> >
> > Actually I don't understand from the comment what Lua module exactly is
> > loaded. Maybe it's better to fix this behaviour?
> What behaviour you want to fix?
That some non-existing module was loaded with `makecmd()`, but since we
have a workaround, maybe it isn't critical.
Anyway, we should do it in a separate patch set.
Feel free to ignore, as I've said.
> > Feel free to ignore.
> >
> >> + LUA_PATH = '',
<snipped>
> >
[1]: https://github.com/tarantool/luajit/blob/f7e5e8abe396411065f8941d04879577e7fae175/test/tarantool-tests/lj-549-bytecode-loader.test.lua#L54
--
Best regards,
Sergey Kaplun
More information about the Tarantool-patches
mailing list