Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: tarantool-patches@freelists.org
Subject: Re: [PATCH 2/3] Add module to ease using Lua iterators from C
Date: Mon, 31 Dec 2018 09:43:56 +0300	[thread overview]
Message-ID: <20181231064355.fsfb7om3p7yqieqv@tkn_work_nb> (raw)
In-Reply-To: <20181226184509.aoreae3ova3k2lz3@esperanza>

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 */

  reply	other threads:[~2018-12-31  6:43 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-16 20:17 [PATCH 0/3] Merger Alexander Turenko
2018-12-16 20:17 ` [PATCH 1/3] Add luaT_iscallable with support of cdata metatype Alexander Turenko
2018-12-26 18:35   ` Vladimir Davydov
2018-12-28  1:46     ` Alexander Turenko
2018-12-28  8:00       ` Vladimir Davydov
2018-12-16 20:17 ` [PATCH 2/3] Add module to ease using Lua iterators from C Alexander Turenko
2018-12-26 18:45   ` Vladimir Davydov
2018-12-31  6:43     ` Alexander Turenko [this message]
2018-12-16 20:17 ` [PATCH 3/3] Add merger for tuple streams Alexander Turenko
2018-12-26 20:11   ` Vladimir Davydov
2019-01-09 21:36     ` Alexander Turenko
2018-12-18 12:16 ` [PATCH 0/3] Merger Alexander Turenko
2019-03-22 14:24 ` [tarantool-patches] [PATCH 0/3] lua: add key_def lua module Kirill Shcherbatov
2019-03-22 16:20   ` Alexander Turenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181231064355.fsfb7om3p7yqieqv@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=vdavydov.dev@gmail.com \
    --subject='Re: [PATCH 2/3] Add module to ease using Lua iterators from C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox