Re: [Tarantool-patches] [PATCH 1/3] merger: drop luaL prefix where contract allows it

[Alexander Turenko <alexander.turenko@tarantool.org>]

On Wed, Jun 03, 2020 at 12:47:45AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> On 01/06/2020 20:10, Alexander Turenko wrote:
> > This change highlights the contract of merge source virtual methods:
> > they don't require a Lua state to be passed with arguments. Internal use
> > of a temporary Lua state is the implementation detail. Technically the
> > functions lean on a Lua state existence in a fiber storage, but the next
> > commit will relax this requirement.
> > 
> > Made the following renames:
> > 
> > * luaL_merge_source_buffer_fetch   -> merge_source_buffer_fetch
> > * luaL_merge_source_buffer_next    -> merge_source_buffer_next
> > * luaL_merge_source_buffer_destroy -> merge_source_buffer_destroy
> > 
> > * keep luaL_merge_source_table_fetch (pass <struct lua_State *>)
> > * luaL_merge_source_table_next     -> merge_source_table_next
> > * luaL_merge_source_table_destroy  -> merge_source_table_destroy
> > 
> > * keep luaL_merge_source_tuple_fetch (change arguments order)
> > * luaL_merge_source_tuple_next     -> merge_source_tuple_next
> > * luaL_merge_source_tuple_destroy  -> merge_source_tuple_destroy
> > 
> > Also added API comments for destroy() and next() virtual methods to
> > uniform them visually with other merge source functions.
> 
> I don't get why do you need these renames. merge_source API is
> located in box/merger.h and box/merger.c. In lua/merger you have
> children of struct merge_source. So they are not merge_source. The
> latter is a virtual struct. lua/merger merge_source structs are
> implementations of this virtual struct. So better not to use the same
> prefix as for the top level merge_source API. IMO.

Not sure I got your idea. We have no special prefix for merge source
implementations except 'merge_source' itself. Say, 'struct
merge_source_buffer' has no prefix. Functions to construct a merge
source from Lua have 'luaL' prefix, because they work on a passed Lua
state.

Functions that use a Lua state (but don't correspond to 'lua_CFunction'
prototype) are prefixed with 'luaL' (maybe will be prefixed with 'luaT'
/ 'luaE' in future if Igor will push us strong enough). The functions of
the question use a Lua state, but find/create it on demand internally.
So they are a kind of usual C functions and should not be prefixed with
luaL. IMO.

I'll CC Igor to see what will look worthful for him.

WBR, Alexander Turenko.