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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi!
Ok, makes sense. LGTM, then, but I really want Sergey to include the
`smoke test` term somewhere in the commit description, so it is obvious,
that we are testing the basic functionality.

On Sun, Aug 06, 2023 at 02:09:27PM +0300, Sergey Kaplun wrote:
> Hi, Max, Sergey!
> The patch is LGTM, after adding the test case (without additional CI
> job, if you don't want), see rational below.
> 
> On 31.07.23, Maxim Kokryashkin wrote:
> > Hi, Sergey!
> 
> <snipped>
> 
> > > tests patch partially.
> > Typo: s/tests the patch/
> > 
> > Well, I've tried to run that test on x32 machine, and nothing happened.
> > I think we should backport this patch without tests then, since this
> > test seems irrelevant to the problem. What do you think?
> > 
> > Also, it is kinda strange, that you are talking about a test in a patch
> > without any tests. You need to either mention that the test is present in
> > the next commit, or move that test here.
> 
> The problem is perfectly repoduced for me if use instructions from [1].
> 
> I suppose that we should add this test, because it:
> 1) Shows the problem for the actual build that can be done by the user.
> 2) Still tests LuaJIT loader.
> 
> > > 
> > > Sergey Bronnikov:
> > > * added the description
> > > ---
> > >  src/lj_bcread.c | 10 +++++-----
> > >  src/lj_lex.c    |  6 ++++++
> > >  src/lj_lex.h    |  1 +
> > >  3 files changed, 12 insertions(+), 5 deletions(-)
> 
> <snipped>
> 
> > > -- 
> > > 2.34.1
> 
> [1]: https://github.com/LuaJIT/LuaJIT/issues/549
> 
> -- 
> Best regards,
> Sergey Kaplun