From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Date: Mon, 31 Dec 2018 09:43:56 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Subject: Re: [PATCH 2/3] Add module to ease using Lua iterators from C
Message-ID: <20181231064355.fsfb7om3p7yqieqv@tkn_work_nb>
References: <cover.1544989900.git.alexander.turenko@tarantool.org>
 <84dde7baf7a3d9364d3199a6eb705c725589bad0.1544989900.git.alexander.turenko@tarantool.org>
 <20181226184509.aoreae3ova3k2lz3@esperanza>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20181226184509.aoreae3ova3k2lz3@esperanza>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Dec 26, 2018 at 09:45:09PM +0300, Vladimir Davydov wrote:
> On Sun, Dec 16, 2018 at 11:17:25PM +0300, Alexander Turenko wrote:
> > Needed for #3276.
> > ---
> >  src/lua/lua_iterator.h | 117 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> >  create mode 100644 src/lua/lua_iterator.h
> > 
> > diff --git a/src/lua/lua_iterator.h b/src/lua/lua_iterator.h
> > new file mode 100644
> > index 000000000..3e1a88c93
> > --- /dev/null
> > +++ b/src/lua/lua_iterator.h
> > @@ -0,0 +1,117 @@
> > +#ifndef TARANTOOL_LUA_LUA_ITERATOR_H_INCLUDED
> > +#define TARANTOOL_LUA_LUA_ITERATOR_H_INCLUDED 1
> 
> Nit: we don't set an include guard macro to any particular value.
> 

I removed this file now, but I fixed guard in merger.h.

> > > > +/**
> > + * This module contains helper functions to interact with a Lua
> > + * iterator from C.
> > + */
> > +
> > +#include <lua.h>
> 
> extern 'C' is missing.
> 

The declarations was moved to utils.h, under extern 'C'.

> > +
> > +/**
> > + * Holds iterator state (references to Lua objects).
> > + */
> > +struct lua_iterator {
> 
> I guess this structure as well as the functions below should have
> luaL_ prefix.
> 

The idea was to name the class 'lua iterator', but now I see it looks
like namespace/prefix 'lua'. Replaced 'lua' with 'luaL'.

> > +	int gen;
> > +	int param;
> > +	int state;
> > +};
> > +
> > +/**
> > + * Create a Lua iterator from {gen, param, state}.
> > + */
> > +struct lua_iterator *
> > +lua_iterator_new_fromtable(lua_State *L, int idx)
> 
> If you include this header into two different source files, you'll get a
> linker error. All functions defined in a header should be marked static
> inline. Anyway, I don't think that defining these functions in a header
> is a good idea - they are heavy and not supposed to be blazing fast.
> 

Ouch, thanks.

> May be, just move them to lua/util.c?

Moved to src/lua/utils.[ch]. I feel 'one class is one module' approach
as kinda more structured, but this is really small one. I wrapped this
functions into {{{ }}} block instead.

> > +/**
> > + * Free all resources hold by the iterator.
> 
> Nit: s/hold/held

Fixed.

> 
> > + */
> > +void lua_iterator_free(lua_State *L, struct lua_iterator *it)
> > +{
> > +	luaL_unref(L, LUA_REGISTRYINDEX, it->gen);
> > +	luaL_unref(L, LUA_REGISTRYINDEX, it->param);
> > +	luaL_unref(L, LUA_REGISTRYINDEX, it->state);
> > +	free(it);
> > +}
> > +
> > +#endif /* TARANTOOL_LUA_LUA_ITERATOR_H_INCLUDED */