From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 8699D42EF5C for ; Tue, 16 Jun 2020 15:08:17 +0300 (MSK) Date: Tue, 16 Jun 2020 14:59:07 +0300 From: Igor Munkin Message-ID: <20200616115907.GV5745@tarantool.org> References: <12754f9b68359f80c41ea655d1a25028e1a72cca.1591028838.git.alexander.turenko@tarantool.org> <20200607165753.mcraz7eh3zhtgpco@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200607165753.mcraz7eh3zhtgpco@tkn_work_nb> 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: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy 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 ) > > > * 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 prefix? I see its common usage for "exported" merger API functions (e.g. , , ) but you also use it for helper routine. How come? * There is a auxiliary function with no Tarantool specifics underneath. Thereby it ought to be named with prefix instead of 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 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 . 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