From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
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 30B7E469710
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  3 Jun 2020 01:47:47 +0300 (MSK)
References: <cover.1591028838.git.alexander.turenko@tarantool.org>
 <12754f9b68359f80c41ea655d1a25028e1a72cca.1591028838.git.alexander.turenko@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <adec4f03-bd1a-0acc-9058-959a6d6e4468@tarantool.org>
Date: Wed, 3 Jun 2020 00:47:45 +0200
MIME-Version: 1.0
In-Reply-To: <12754f9b68359f80c41ea655d1a25028e1a72cca.1591028838.git.alexander.turenko@tarantool.org>
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 <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

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.