From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <alexander.turenko@tarantool.org>
Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112])
 (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 3C128469719
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 14 Oct 2020 06:50:20 +0300 (MSK)
Date: Wed, 14 Oct 2020 06:50:39 +0300
From: Alexander Turenko <alexander.turenko@tarantool.org>
Message-ID: <20201014035039.glx2veliqncoyp5z@tkn_work_nb>
References: <cover.1602629628.git.tsafin@tarantool.org>
 <fefe53d5e5c88be9e6762783003a1213b2d67cda.1602629628.git.tsafin@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <fefe53d5e5c88be9e6762783003a1213b2d67cda.1602629628.git.tsafin@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 2.X v4 3/4] module api: introduced
 luaT_toibuf instead of luaL_checkibuf
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: Timur Safin <tsafin@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@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().