From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 A84AB42EF5C for ; Thu, 11 Jun 2020 19:17:33 +0300 (MSK) References: <12754f9b68359f80c41ea655d1a25028e1a72cca.1591028838.git.alexander.turenko@tarantool.org> <20200607165753.mcraz7eh3zhtgpco@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: <34a5c573-c6ad-b534-912e-7c9a7151311e@tarantool.org> Date: Thu, 11 Jun 2020 18:17:31 +0200 MIME-Version: 1.0 In-Reply-To: <20200607165753.mcraz7eh3zhtgpco@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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 ) >>> * 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 _. 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.