Yegappan Lakshmanan

unread,

Jul 28, 2021, 10:53:35 PM7/28/21

to vim/vim, Subscribed

Fix for #8648.

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/8655

Commit Summary

Use a grow array for Lua print

File Changes

Patch Links:

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

codecov[bot]

unread,

Jul 28, 2021, 11:00:59 PM7/28/21

to vim/vim, Subscribed

Codecov Report

Merging #8655 (f7484fb) into master (0732932) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@

##           master    #8655      +/-   ##

==========================================

- Coverage    2.46%    2.46%   -0.01%     

==========================================

  Files         148      148              

  Lines      164959   164969      +10     

==========================================

  Hits         4072     4072              

- Misses     160887   160897      +10

Flag	Coverage Δ
huge-gcc-unittests	`2.46% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/if_lua.c	`0.00% <0.00%> (ø)`
src/misc2.c	`5.31% <0.00%> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0732932...f7484fb. Read the comment docs.

Martin Tournoij

unread,

Jul 29, 2021, 12:01:07 AM7/29/21

to vim/vim, Subscribed

Hm, the Lua manual doesn't mention this at all; luaL_buffinit() does use LUAL_BUFFERSIZE initially, but luaL_addlstring() calls prepbuffsize() which grows it as-needed.

I mean, if it works then it works so it's fine, but I think it was somehow using the API wrong, unless I completely misunderstood things.

Bram Moolenaar

unread,

Jul 29, 2021, 2:22:55 PM7/29/21

to vim/vim, Subscribed

Closed #8655 via 41114a2.

Yegappan Lakshmanan

unread,

Jul 29, 2021, 10:33:05 PM7/29/21

to vim_dev, reply+ACY5DGBPW2YCVI6GWZ...@reply.github.com, vim/vim, Subscribed

Hi,

On Wed, Jul 28, 2021 at 9:01 PM Martin Tournoij <vim-dev...@256bit.org> wrote:

Hm, the Lua manual doesn't mention this at all; luaL_buffinit() does use LUAL_BUFFERSIZE initially, but luaL_addlstring() calls prepbuffsize() which grows it as-needed.

I mean, if it works then it works so it's fine, but I think it was somehow using the API wrong, unless I completely misunderstood things.

The string returned by lua_tolstring() is on the Lua stack

and using the Lua buffer also modifies the stack. I think using the

Lua buffer while holding a pointer to the value returned by

lua_tolstring() leads to this problem.

The Lua reference manual has the following:

==============================================

Because Lua has garbage collection, there is no guarantee that the
pointer returned by lua_tolstring will be valid after the corresponding
Lua value is removed from the stack.

During its normal operation, a string buffer uses a variable number of
stack slots. So, while using a buffer, you cannot assume that you know
where the top of the stack is. You can use the stack between successive
calls to buffer operations as long as that use is balanced; that is,
when you call a buffer operation, the stack is at the same level it was
immediately after the previous buffer operation.

==============================================

I tried using vim_strsave() to create a copy of the string returned

by lua_tolstring(). This fixes the issue. But this string needs to be

allocated and freed once per loop iteration. It is more efficient

to use the grow array to store the string.

Regards,

Yegappan

vim-dev ML

unread,

Jul 29, 2021, 10:33:23 PM7/29/21

to vim/vim, vim-dev ML, Your activity

Hi,

On Wed, Jul 28, 2021 at 9:01 PM Martin Tournoij ***@***.***>

wrote:

> Hm, the Lua manual

> <https://www.lua.org/manual/5.4/manual.html#luaL_Buffer> doesn't mention

Martin Tournoij

unread,

Jul 30, 2021, 3:23:04 AM7/30/21

to vim/vim, vim-dev ML, Comment

Ah yes, that makes sense.

For some strange reason I dreamt about this issue last night and from what I recall in my dream I came to the same conclusion. The human mind works in funny ways 😅

—
You are receiving this because you commented.

Bram Moolenaar

unread,

Jul 30, 2021, 5:54:18 AM7/30/21

to vim...@googlegroups.com, Yegappan Lakshmanan, reply+ACY5DGBPW2YCVI6GWZ...@reply.github.com

> On Wed, Jul 28, 2021 at 9:01 PM Martin Tournoij <vim-dev...@256bit.org>
> wrote:
>
> > Hm, the Lua manual

> > <https://www.lua.org/manual/5.4/manual.html#luaL_Buffer> doesn't mention

So, another way would have been to put the pop outside of the loop,
clean up afterwards.

I prefer using the growarray anyway, having to carefully read Lua
documentation to understand how this works has its disadvantages, and
apparently leads to mistakes. Well, using a growarray also has this
catch, that a pointer into the data becomes invalid when the data is
reallocated.

--
The greatest lies of all time:
(1) The check is in the mail.
(2) We have a really challenging assignment for you.
(3) I love you.
(4) All bugs have been fixed.
(5) This won't hurt a bit.
(6) Honey, I just need to debug this program and be home in 5 minutes.
(7) I have just sent you an e-mail about that.
(8) Of course I'll respect you in the morning.
(9) I'm from the government, and I'm here to help you.

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

vim-dev ML

unread,

Jul 30, 2021, 5:54:33 AM7/30/21

to vim/vim, vim-dev ML, Your activity

> On Wed, Jul 28, 2021 at 9:01 PM Martin Tournoij ***@***.***>

/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\

/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

Yegappan Lakshmanan

unread,

Jul 30, 2021, 10:40:35 AM7/30/21

to Bram Moolenaar, vim_dev, reply+ACY5DGBPW2YCVI6GWZ...@reply.github.com

Hi Bram,

The pop is used to remove the value returned by Lua 'tostring' for
each argument. So the pop has to be within the loop. If it is moved
outside the loop, then we need to pop multiple values.

Also, based on the documentation, it looks like Lua expects the
stack to be the same between successive Lua buffer operations.
This means that we cannot store and use the string returned by
tostring in the stack as the Lua buffer operations are performed
a few times within the loop.

Regards,
Yegappan

vim-dev ML

unread,

Jul 30, 2021, 10:40:51 AM7/30/21

to vim/vim, vim-dev ML, Your activity

Hi Bram,

On Fri, Jul 30, 2021 at 2:54 AM Bram Moolenaar ***@***.***> wrote:
>
>
> > On Wed, Jul 28, 2021 at 9:01 PM Martin Tournoij ***@***.***>

Reply all

Reply to author

Forward

[vim/vim] Use a grow array for Lua print (#8655)

Yegappan Lakshmanan

You can view, comment on, or merge this pull request online at:

Commit Summary

File Changes

Patch Links:

codecov[bot]

Codecov Report

Martin Tournoij

Bram Moolenaar

Yegappan Lakshmanan

vim-dev ML

Martin Tournoij

Bram Moolenaar

vim-dev ML

Yegappan Lakshmanan

vim-dev ML