Re: [Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced luaT_toibuf instead of luaL_checkibuf

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

> module api: introduced luaT_toibuf instead of luaL_checkibuf

We usually try to fit into 50 symbols. Okay, sometimes we overrun it a
bit (and I was one of persons who asked to don't make it hard limit),
where it is hard to give a short description, but it is not the case,
right?

BTW, imperative mood ('introduce') is suggested for a commit message
header (just header, not the entire body).

(I would name it 'module api/lua: expose luaT_toibuf()' if you want my
opinion on this essential topic.)

> * made `luaL_checkibuf` public;
> * renamed it to `luaT_toibuf` to follow naming convention
>   (it's not raising error, and is casting to ibuf type).

Did you mean 'returns a pointer to' by 'is casting to'? It tooks some
time to get the idea. Hmm, but lua_check<...> also returns a pointer.

I don't push you to anything, just noted that I'm a bit confused here as
a reader.

> +static int
> +test_checkibuf(lua_State *L)
> +{
> +	struct ibuf *buf;
> +	buf = luaT_toibuf(L, -1);
> +	lua_pushboolean(L, buf != NULL);
> +	return 1;
> +}

I don't bother enough about the test to insist on it now, but it looks
just as inconsistency. Why not test_toibuf()?

> +local function test_buffers(test, module)
> +    test:plan(9)
> +    local ffi = require('ffi')
> +    local buffer = require('buffer')
> +
> +    local bufalloc = buffer.static_alloc("char", 128)
> +    local ibuf = buffer.ibuf()
> +    local pbuf = ibuf:alloc(128)
> +
> +    test:ok(not module.checkibuf(nil), 'checkibuf of nil')
> +    test:ok(not module.checkibuf({}), 'checkibuf of {}')
> +    test:ok(not module.checkibuf(1LL), 'checkibuf of 1LL')
> +    test:ok(not module.checkibuf(box.NULL), 'checkibuf of box.NULL')
> +    test:ok(not module.checkibuf(buffer.reg1), 'checkibuf of reg1')
> +    test:ok(not module.checkibuf(bufalloc), 'checkibuf of allocated buffer')
> +    test:ok(module.checkibuf(buffer.IBUF_SHARED), "checkibuf of ibuf*")
> +    test:ok(module.checkibuf(ibuf), 'checkibuf of ibuf')
> +    test:ok(not module.checkibuf(pbuf), 'checkibuf of pointer to ibuf data')

And here too: why 'checkibuf of nil'? The subject of the test is
luaT_isibuf().