Re: [Tarantool-patches] [PATCH luajit 1/2][v2] Fix embedded bytecode loader.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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