From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 74FD1469710 for ; Sun, 7 Jun 2020 19:58:08 +0300 (MSK) Date: Sun, 7 Jun 2020 19:57:53 +0300 From: Alexander Turenko Message-ID: <20200607165753.mcraz7eh3zhtgpco@tkn_work_nb> References: <12754f9b68359f80c41ea655d1a25028e1a72cca.1591028838.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH 1/3] merger: drop luaL prefix where contract allows it List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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 ) > > * 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.