Re: [PATCH v2 2/6] Add functions to ease using Lua iterators from C

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thanks for the review!

I commented inline and fixed all comments except one, where I doubt.

The patch from the previous version at end of the email.

List of changes:

* Fixed zero gen() retvals case.
* Added a unit test.
* Removed unneded cast from (void *).
* Allow to pass values w/o a table.
  - Also removed _fromtable suffix from luaL_iterator_new().
* luaL_iterator_free() -> luaL_iterator_delete().

WBR, Alexander Turenko.

On Thu, Jan 10, 2019 at 03:29:09PM +0300, Vladimir Davydov wrote:
> On Wed, Jan 09, 2019 at 11:20:10PM +0300, Alexander Turenko wrote:
> > Needed for #3276.
> 
> Again, I'm not quite sure that you'll need this patch after you
> rework the merger API so I'm not applying it until you send the
> new API proposal.

If we'll support iterator sources those helpers are convenient. All APIs
we discussing now have them.

IMHO, it is possible, but unlikely we'll decide to get rid of them.

> 
> > ---
> >  src/lua/utils.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/lua/utils.h | 28 +++++++++++++++++++++
> 
> Some tests would be nice to have.

I have added test/unit/luaL_iterator.c. I have to link many parts (*.a
libraries) of tarantool to it and system dynamic libraries (dependencies
of *.a), but I think size does not matter much here.

This test found one error! An iterator can return zero count of values
instead of returning nil and ipairs() behaves in that way. I fixed the
implementation.

> 
> >  2 files changed, 94 insertions(+)
> > 
> > diff --git a/src/lua/utils.c b/src/lua/utils.c
> > index eefb860ee..4d1eee6ab 100644
> > --- a/src/lua/utils.c
> > +++ b/src/lua/utils.c
> > @@ -969,6 +969,72 @@ luaT_state(void)
> >  	return tarantool_L;
> >  }
> >  
> > +/* {{{ Helper functions to interact with a Lua iterator from C */
> > +
> > +struct luaL_iterator {
> > +	int gen;
> > +	int param;
> > +	int state;
> > +};
> > +
> > +struct luaL_iterator *
> > +luaL_iterator_new_fromtable(lua_State *L, int idx)
> > +{
> > +	struct luaL_iterator *it = (struct luaL_iterator *) malloc(
> 
> Nit: no need to convert void * to struct luaL_iterator *.

Yep. Fixed.

> 
> > +		sizeof(struct luaL_iterator));
> > +
> > +	lua_rawgeti(L, idx, 1); /* Popped by luaL_ref(). */
> > +	it->gen = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	lua_rawgeti(L, idx, 2); /* Popped by luaL_ref(). */
> > +	it->param = luaL_ref(L, LUA_REGISTRYINDEX);
> > +	lua_rawgeti(L, idx, 3); /* Popped by luaL_ref(). */
> > +	it->state = luaL_ref(L, LUA_REGISTRYINDEX);
> > +
> > +	return it;
> > +}
> > +
> > +int
> > +luaL_iterator_next(lua_State *L, struct luaL_iterator *it)
> > +{
> > +	int frame_start = lua_gettop(L);
> > +
> > +	/* Call gen(param, state). */
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, it->gen);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, it->param);
> > +	lua_rawgeti(L, LUA_REGISTRYINDEX, it->state);
> > +	lua_call(L, 2, LUA_MULTRET);
> > +	int nresults = lua_gettop(L) - frame_start;
> > +	if (nresults == 0) {
> > +		luaL_error(L, "luaL_iterator_next: gen(param, state) must "
> > +			      "return at least one result");
> > +		unreachable();
> > +		return 0;
> > +	}
> > +
> > +	/* The call above returns nil as the first result. */
> > +	if (lua_isnil(L, frame_start + 1)) {
> > +		lua_settop(L, frame_start);
> > +		return 0;
> > +	}
> > +
> > +	/* Save the first result to it->state. */
> > +	luaL_unref(L, LUA_REGISTRYINDEX, it->state);
> > +	lua_pushvalue(L, frame_start + 1); /* Popped by luaL_ref(). */
> > +	it->state = luaL_ref(L, LUA_REGISTRYINDEX);
> > +
> > +	return nresults;
> > +}
> > +
> > +void luaL_iterator_free(lua_State *L, struct luaL_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);
> > +}
> > +
> > +/* }}} */
> > +
> >  int
> >  tarantool_lua_utils_init(struct lua_State *L)
> >  {
> > diff --git a/src/lua/utils.h b/src/lua/utils.h
> > index bd302d8e9..6ba2e4767 100644
> > --- a/src/lua/utils.h
> > +++ b/src/lua/utils.h
> > @@ -525,6 +525,34 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
> >  		luaL_error(L, "number must not be NaN or Inf");
> >  }
> >  
> > +/* {{{ Helper functions to interact with a Lua iterator from C */
> > +
> > +/**
> > + * Holds iterator state (references to Lua objects).
> > + */
> > +struct luaL_iterator;
> 
> I'd make luaL_iterator struct transparent so that one could define it
> on stack.
> 

But luaL_iterator_new() do malloc and return a pointer. So we need to
change the function or add another one to initialize a structure
allocated outsize of the module.

Can you please suggest me how the API should look if we'll make the
structure transparent?

I left it unchanged until we'll discuss this aspect.

> > +
> > +/**
> > + * Create a Lua iterator from {gen, param, state}.
> 
> May be, we could pass idx == 0 to create an iterator from
> gen, param, state (without a table)? Would it be worthwhile?
> 

I think it is good idea, because cases could be different. My thought
was that we'll add another function for this case (if we'll need), but
using idx == 0 is better. I created the similar API before for
luaT_newtuple().

And I think the new merger API will require it.

Added.

> > + */
> > +struct luaL_iterator *
> > +luaL_iterator_new_fromtable(lua_State *L, int idx);
> 
> I don't think that _fromtable suffix is really necessary.
> 

In light of the idx == 0 support, yep. Fixed.

> > +
> > +/**
> > + * Move iterator to the next value. Push values returned by
> > + * gen(param, state) and return its count. Zero means no more
> > + * results available.
> > + */
> > +int
> > +luaL_iterator_next(lua_State *L, struct luaL_iterator *it);
> > +
> > +/**
> > + * Free all resources held by the iterator.
> > + */
> > +void luaL_iterator_free(lua_State *L, struct luaL_iterator *it);
> 
> We usually match _new with _delete.

Fixed.

> 
> > +
> > +/* }}} */
> > +
> >  int
> >  tarantool_lua_utils_init(struct lua_State *L);

----

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 4d1eee6ab..173d59a59 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -978,17 +978,30 @@ struct luaL_iterator {
 };
 
 struct luaL_iterator *
-luaL_iterator_new_fromtable(lua_State *L, int idx)
+luaL_iterator_new(lua_State *L, int idx)
 {
-	struct luaL_iterator *it = (struct luaL_iterator *) malloc(
-		sizeof(struct luaL_iterator));
-
-	lua_rawgeti(L, idx, 1); /* Popped by luaL_ref(). */
-	it->gen = luaL_ref(L, LUA_REGISTRYINDEX);
-	lua_rawgeti(L, idx, 2); /* Popped by luaL_ref(). */
-	it->param = luaL_ref(L, LUA_REGISTRYINDEX);
-	lua_rawgeti(L, idx, 3); /* Popped by luaL_ref(). */
-	it->state = luaL_ref(L, LUA_REGISTRYINDEX);
+	struct luaL_iterator *it = malloc(sizeof(struct luaL_iterator));
+
+	if (idx == 0) {
+		/* gen, param, state are on top of a Lua stack. */
+		lua_pushvalue(L, -3); /* Popped by luaL_ref(). */
+		it->gen = luaL_ref(L, LUA_REGISTRYINDEX);
+		lua_pushvalue(L, -2); /* Popped by luaL_ref(). */
+		it->param = luaL_ref(L, LUA_REGISTRYINDEX);
+		lua_pushvalue(L, -1); /* Popped by luaL_ref(). */
+		it->state = luaL_ref(L, LUA_REGISTRYINDEX);
+	} else {
+		/*
+		 * {gen, param, state} table is at idx in a Lua
+		 * stack.
+		 */
+		lua_rawgeti(L, idx, 1); /* Popped by luaL_ref(). */
+		it->gen = luaL_ref(L, LUA_REGISTRYINDEX);
+		lua_rawgeti(L, idx, 2); /* Popped by luaL_ref(). */
+		it->param = luaL_ref(L, LUA_REGISTRYINDEX);
+		lua_rawgeti(L, idx, 3); /* Popped by luaL_ref(). */
+		it->state = luaL_ref(L, LUA_REGISTRYINDEX);
+	}
 
 	return it;
 }
@@ -1004,15 +1017,15 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it)
 	lua_rawgeti(L, LUA_REGISTRYINDEX, it->state);
 	lua_call(L, 2, LUA_MULTRET);
 	int nresults = lua_gettop(L) - frame_start;
-	if (nresults == 0) {
-		luaL_error(L, "luaL_iterator_next: gen(param, state) must "
-			      "return at least one result");
-		unreachable();
-		return 0;
-	}
 
-	/* The call above returns nil as the first result. */
-	if (lua_isnil(L, frame_start + 1)) {
+	/*
+	 * gen() function can either return nil when the iterator
+	 * ends or return zero count of values.
+	 *
+	 * In LuaJIT pairs() returns nil, but ipairs() returns
+	 * nothing when ends.
+	 */
+	if (nresults == 0 || lua_isnil(L, frame_start + 1)) {
 		lua_settop(L, frame_start);
 		return 0;
 	}
@@ -1025,7 +1038,7 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it)
 	return nresults;
 }
 
-void luaL_iterator_free(lua_State *L, struct luaL_iterator *it)
+void luaL_iterator_delete(lua_State *L, struct luaL_iterator *it)
 {
 	luaL_unref(L, LUA_REGISTRYINDEX, it->gen);
 	luaL_unref(L, LUA_REGISTRYINDEX, it->param);
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 6ba2e4767..2df2f5015 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -533,10 +533,16 @@ luaL_checkfinite(struct lua_State *L, struct luaL_serializer *cfg,
 struct luaL_iterator;
 
 /**
- * Create a Lua iterator from {gen, param, state}.
+ * Create a Lua iterator from a gen, param, state triplet.
+ *
+ * If idx == 0, then three top stack values are used as the
+ * triplet.
+ *
+ * Otherwise idx is index on Lua stack points to a
+ * {gen, param, state} table.
  */
 struct luaL_iterator *
-luaL_iterator_new_fromtable(lua_State *L, int idx);
+luaL_iterator_new(lua_State *L, int idx);
 
 /**
  * Move iterator to the next value. Push values returned by
@@ -549,7 +555,7 @@ luaL_iterator_next(lua_State *L, struct luaL_iterator *it);
 /**
  * Free all resources held by the iterator.
  */
-void luaL_iterator_free(lua_State *L, struct luaL_iterator *it);
+void luaL_iterator_delete(lua_State *L, struct luaL_iterator *it);
 
 /* }}} */
 
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 0025d3611..c2c45a4b8 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -138,6 +138,10 @@ add_executable(histogram.test histogram.c)
 target_link_libraries(histogram.test stat unit)
 add_executable(ratelimit.test ratelimit.c)
 target_link_libraries(ratelimit.test unit)
+add_executable(luaL_iterator.test luaL_iterator.c)
+target_link_libraries(luaL_iterator.test unit server core misc
+    ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
+    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
 
 add_executable(say.test say.c)
 target_link_libraries(say.test core unit)
diff --git a/test/unit/luaL_iterator.c b/test/unit/luaL_iterator.c
new file mode 100644
index 000000000..5a254f27d
--- /dev/null
+++ b/test/unit/luaL_iterator.c
@@ -0,0 +1,159 @@
+#include <lua.h>       /* lua_*() */
+#include <lauxlib.h>   /* luaL_*() */
+#include <lualib.h>    /* luaL_openlibs() */
+#include "unit.h"      /* plan, header, footer, is */
+#include "lua/utils.h" /* luaL_iterator_*() */
+
+extern char fun_lua[];
+
+int
+main()
+{
+	struct {
+		/* A string to output with a test case. */
+		const char *description;
+		/* A string with Lua code to push an iterator. */
+		const char *init;
+		/*
+		 * How much values are pushed by the Lua code
+		 * above.
+		 */
+		int init_retvals;
+		/*
+		 * Start values from this number to distinguish
+		 * them from its ordinal.
+		 */
+		int first_value;
+		/*
+		 * Lua stack index where {gen, param, state} is
+		 * placed or zero.
+		 */
+		int idx;
+		/* How much values are in the iterator. */
+		int iterations;
+	} cases[] = {
+		{
+			.description = "pairs, zero idx",
+			.init = "return pairs({42})",
+			.init_retvals = 3,
+			.first_value = 42,
+			.idx = 0,
+			.iterations = 1,
+		},
+		{
+			.description = "ipairs, zero idx",
+			.init = "return ipairs({42, 43, 44})",
+			.init_retvals = 3,
+			.first_value = 42,
+			.idx = 0,
+			.iterations = 3,
+		},
+		{
+			.description = "luafun iterator, zero idx",
+			.init = "return fun.wrap(ipairs({42, 43, 44}))",
+			.init_retvals = 3,
+			.first_value = 42,
+			.idx = 0,
+			.iterations = 3,
+		},
+		{
+			.description = "pairs, from table",
+			.init = "return {pairs({42})}",
+			.init_retvals = 1,
+			.first_value = 42,
+			.idx = -1,
+			.iterations = 1,
+		},
+		{
+			.description = "ipairs, from table",
+			.init = "return {ipairs({42, 43, 44})}",
+			.init_retvals = 1,
+			.first_value = 42,
+			.idx = -1,
+			.iterations = 3,
+		},
+		{
+			.description = "luafun iterator, from table",
+			.init = "return {fun.wrap(ipairs({42, 43, 44}))}",
+			.init_retvals = 1,
+			.first_value = 42,
+			.idx = -1,
+			.iterations = 3,
+		},
+	};
+
+	int cases_cnt = (int) (sizeof(cases) / sizeof(cases[0]));
+	/*
+	 * * Check stack size after creating luaL_iterator (triple
+	 *   times).
+	 * * 4 checks per iteration.
+	 * * Check that values ends.
+	 */
+	int planned = 0;
+	for (int i = 0; i < cases_cnt; ++i)
+		planned += cases[i].iterations * 4 + 4;
+
+	plan(planned);
+	header();
+
+	struct lua_State *L = luaL_newstate();
+	luaL_openlibs(L);
+
+	/*
+	 * Expose luafun.
+	 *
+	 * Don't register it in package.loaded for simplicity.
+	 */
+	luaL_loadstring(L, fun_lua);
+	lua_call(L, 0, 1);
+	lua_setglobal(L, "fun");
+
+	for (int i = 0; i < cases_cnt; ++i) {
+		const char *description = cases[i].description;
+		int top = lua_gettop(L);
+
+		/* Push an iterator to the Lua stack. */
+		luaL_loadstring(L, cases[i].init);
+		lua_call(L, 0, cases[i].init_retvals);
+
+		/* Create the luaL_iterator structure. */
+		struct luaL_iterator *it = luaL_iterator_new(L, cases[i].idx);
+		lua_pop(L, cases[i].init_retvals);
+
+		/* Check stack size. */
+		is(lua_gettop(L) - top, 0, "%s: stack size", description);
+
+		/* Iterate over values and check them. */
+		for (int j = 0; j < cases[i].iterations; ++j) {
+			int top = lua_gettop(L);
+			int rc = luaL_iterator_next(L, it);
+			is(rc, 2, "%s: iter %d: gen() retval count",
+			   description, j);
+			is(luaL_checkinteger(L, -2), j + 1,
+			   "%s: iter %d: gen() 1st retval",
+			   description, j);
+			is(luaL_checkinteger(L, -1), j + cases[i].first_value,
+			   "%s: iter %d: gen() 2nd retval",
+			   description, j);
+			lua_pop(L, 2);
+			is(lua_gettop(L) - top, 0, "%s: iter: %d: stack size",
+			   description, j);
+		}
+
+		/* Check the iterator ends when expected. */
+		int rc = luaL_iterator_next(L, it);
+		is(rc, 0, "%s: iterator ends", description);
+
+		/* Check stack size. */
+		is(lua_gettop(L) - top, 0, "%s: stack size", description);
+
+		/* Free the luaL_iterator structure. */
+		luaL_iterator_delete(L, it);
+
+		/* Check stack size. */
+		is(lua_gettop(L) - top, 0, "%s: stack size", description);
+	}
+
+	footer();
+	return check_plan();
+}
diff --git a/test/unit/luaL_iterator.result b/test/unit/luaL_iterator.result
new file mode 100644
index 000000000..f4eda5695
--- /dev/null
+++ b/test/unit/luaL_iterator.result
@@ -0,0 +1,83 @@
+1..80
+	*** main ***
+ok 1 - pairs, zero idx: stack size
+ok 2 - pairs, zero idx: iter 0: gen() retval count
+ok 3 - pairs, zero idx: iter 0: gen() 1st retval
+ok 4 - pairs, zero idx: iter 0: gen() 2nd retval
+ok 5 - pairs, zero idx: iter: 0: stack size
+ok 6 - pairs, zero idx: iterator ends
+ok 7 - pairs, zero idx: stack size
+ok 8 - pairs, zero idx: stack size
+ok 9 - ipairs, zero idx: stack size
+ok 10 - ipairs, zero idx: iter 0: gen() retval count
+ok 11 - ipairs, zero idx: iter 0: gen() 1st retval
+ok 12 - ipairs, zero idx: iter 0: gen() 2nd retval
+ok 13 - ipairs, zero idx: iter: 0: stack size
+ok 14 - ipairs, zero idx: iter 1: gen() retval count
+ok 15 - ipairs, zero idx: iter 1: gen() 1st retval
+ok 16 - ipairs, zero idx: iter 1: gen() 2nd retval
+ok 17 - ipairs, zero idx: iter: 1: stack size
+ok 18 - ipairs, zero idx: iter 2: gen() retval count
+ok 19 - ipairs, zero idx: iter 2: gen() 1st retval
+ok 20 - ipairs, zero idx: iter 2: gen() 2nd retval
+ok 21 - ipairs, zero idx: iter: 2: stack size
+ok 22 - ipairs, zero idx: iterator ends
+ok 23 - ipairs, zero idx: stack size
+ok 24 - ipairs, zero idx: stack size
+ok 25 - luafun iterator, zero idx: stack size
+ok 26 - luafun iterator, zero idx: iter 0: gen() retval count
+ok 27 - luafun iterator, zero idx: iter 0: gen() 1st retval
+ok 28 - luafun iterator, zero idx: iter 0: gen() 2nd retval
+ok 29 - luafun iterator, zero idx: iter: 0: stack size
+ok 30 - luafun iterator, zero idx: iter 1: gen() retval count
+ok 31 - luafun iterator, zero idx: iter 1: gen() 1st retval
+ok 32 - luafun iterator, zero idx: iter 1: gen() 2nd retval
+ok 33 - luafun iterator, zero idx: iter: 1: stack size
+ok 34 - luafun iterator, zero idx: iter 2: gen() retval count
+ok 35 - luafun iterator, zero idx: iter 2: gen() 1st retval
+ok 36 - luafun iterator, zero idx: iter 2: gen() 2nd retval
+ok 37 - luafun iterator, zero idx: iter: 2: stack size
+ok 38 - luafun iterator, zero idx: iterator ends
+ok 39 - luafun iterator, zero idx: stack size
+ok 40 - luafun iterator, zero idx: stack size
+ok 41 - pairs, from table: stack size
+ok 42 - pairs, from table: iter 0: gen() retval count
+ok 43 - pairs, from table: iter 0: gen() 1st retval
+ok 44 - pairs, from table: iter 0: gen() 2nd retval
+ok 45 - pairs, from table: iter: 0: stack size
+ok 46 - pairs, from table: iterator ends
+ok 47 - pairs, from table: stack size
+ok 48 - pairs, from table: stack size
+ok 49 - ipairs, from table: stack size
+ok 50 - ipairs, from table: iter 0: gen() retval count
+ok 51 - ipairs, from table: iter 0: gen() 1st retval
+ok 52 - ipairs, from table: iter 0: gen() 2nd retval
+ok 53 - ipairs, from table: iter: 0: stack size
+ok 54 - ipairs, from table: iter 1: gen() retval count
+ok 55 - ipairs, from table: iter 1: gen() 1st retval
+ok 56 - ipairs, from table: iter 1: gen() 2nd retval
+ok 57 - ipairs, from table: iter: 1: stack size
+ok 58 - ipairs, from table: iter 2: gen() retval count
+ok 59 - ipairs, from table: iter 2: gen() 1st retval
+ok 60 - ipairs, from table: iter 2: gen() 2nd retval
+ok 61 - ipairs, from table: iter: 2: stack size
+ok 62 - ipairs, from table: iterator ends
+ok 63 - ipairs, from table: stack size
+ok 64 - ipairs, from table: stack size
+ok 65 - luafun iterator, from table: stack size
+ok 66 - luafun iterator, from table: iter 0: gen() retval count
+ok 67 - luafun iterator, from table: iter 0: gen() 1st retval
+ok 68 - luafun iterator, from table: iter 0: gen() 2nd retval
+ok 69 - luafun iterator, from table: iter: 0: stack size
+ok 70 - luafun iterator, from table: iter 1: gen() retval count
+ok 71 - luafun iterator, from table: iter 1: gen() 1st retval
+ok 72 - luafun iterator, from table: iter 1: gen() 2nd retval
+ok 73 - luafun iterator, from table: iter: 1: stack size
+ok 74 - luafun iterator, from table: iter 2: gen() retval count
+ok 75 - luafun iterator, from table: iter 2: gen() 1st retval
+ok 76 - luafun iterator, from table: iter 2: gen() 2nd retval
+ok 77 - luafun iterator, from table: iter: 2: stack size
+ok 78 - luafun iterator, from table: iterator ends
+ok 79 - luafun iterator, from table: stack size
+ok 80 - luafun iterator, from table: stack size
+	*** main: done ***