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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

On 07/06/2020 18:57, Alexander Turenko wrote:
> 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.

But merge_source_buffer != merge_source. Generally we name methods
using <struct name>_<method_name>. So the correct way was to use
merge_source_buffer_ prefix. Not just merge_source_. Talking of luaL_
prefix, IMO its role here is secondary. You just need to either use
it for all merge_source_buffer methods as an additional prefix, or
don't use it in either of them.

Currently your merge_source_buffer methods are:

    merge_source_buffer_destroy
    merge_source_buffer_next
    luaL_merge_source_buffer_new
    luaL_merge_source_buffer_fetch_impl
    merge_source_buffer_fetch

Seems inconsistent. Both names and argument order in some of them.
If a function takes luaL_State, it does not take luaL_ prefix
automatically and unconditionally. It is just a parameter. These
functions in the first place are merge_source_buffer methods. Not
luaL_State methods.