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

[Igor Munkin <imun@tarantool.org>]

Sasha,

On 07.06.20, 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.
> 

As for me I see a mess with the naming already exists:
* What is the rule for using <lbox_> prefix? I see its common usage for
  "exported" merger API functions (e.g. <lbox_merger_new_buffer_source>,
  <lbox_merger_new_table_source>, <lbox_merger_new_tuple_source>) but
  you also use it for <lbox_merge_source_new> helper routine. How come?
* There is a <luaT_check_merge_source> auxiliary function with no
  Tarantool specifics underneath. Thereby it ought to be named with
  <luaL_> prefix instead of <luaT_> one as you mentioned here[1]. Looks
  like another naming rule "violation", doesn't it? However, you left
  this one unchanged.

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

Well, let's consider <luaL_addvalue> function from lauxlib.h[2], that
has the following signature:
| LUALIB_API void luaL_addvalue(luaL_Buffer *B)
It neither corresponds lua_CFunction prototype, nor has explicit
lua_State argument in signature, *but* yet is prefixed with <luaL_>. It
uses lua_State from luaL_Buffer that is quite similar usage to the way
you obtain lua_State for merger needs.

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

I'm not fussy here (it's an old code) and we don't have strict naming
policy for now, so let's discard these changes until we made any
decision regarding #4577.

> 
> WBR, Alexander Turenko.

[1]: https://github.com/tarantool/tarantool/issues/4577
[2]: https://www.lua.org/manual/5.1/manual.html#luaL_addvalue

-- 
Best regards,
IM