From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 5886446970E for ; Fri, 10 Jan 2020 18:51:31 +0300 (MSK) References: <20191111145009.91425-1-olegrok@tarantool.org> <20200110123205.fu5kg376hxcq2n2f@tkn_work_nb> From: Oleg Babin Message-ID: <5c9365fc-0e7f-4363-2a17-9449cf45eaec@tarantool.org> Date: Fri, 10 Jan 2020 18:51:30 +0300 MIME-Version: 1.0 In-Reply-To: <20200110123205.fu5kg376hxcq2n2f@tkn_work_nb> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2] error: Add __concat method to error object List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.tarantool.org Thanks for your review! As we discussed verbally I improve test and consider cases of tables and ffi objects We will not overcomplicate logic with proper error message construction: user will see "attempt to concatenate ''type' and ''string''' " On 10/01/2020 15:32, Alexander Turenko wrote: > On Mon, Nov 11, 2019 at 05:50:09PM +0300, Oleg Babin wrote: >> From: Oleg Babin >> >> Usually functions return pair {nil, err} and expected that err is string. >> Let's make the behaviour of error object closer to string >> and define __concat metamethod. >> >> Closes #4489 >> --- >> >> Changes in v2: >> - Added tests >> >> --- >> src/lua/error.lua | 8 +++++++ >> test/box/misc.result | 47 +++++++++++++++++++++++++++++++++++++++++- >> test/box/misc.test.lua | 17 ++++++++++++++- >> 3 files changed, 70 insertions(+), 2 deletions(-) >> >> diff --git a/src/lua/error.lua b/src/lua/error.lua >> index 28fc0377d..b44fab7b6 100644 >> --- a/src/lua/error.lua >> +++ b/src/lua/error.lua >> @@ -150,9 +150,17 @@ local function error_index(err, key) >> return error_methods[key] >> end >> >> +local function error_concat(lhs, rhs) >> + if lhs == nil or rhs == nil then >> + error("attempt to concatenate struct error and nil") > Nit: If we want to follow string concatenation errors, then it should be > 'struct error and nil' or 'nil and struct error' depending of what > values lhs and rhs have. See below possible resolution. > >> + end >> + return tostring(lhs) .. tostring(rhs) >> +end >> + >> local error_mt = { >> __index = error_index; >> __tostring = error_message; >> + __concat = error_concat; >> }; >> >> ffi.metatype('struct error', error_mt); > It is the same as > https://github.com/tarantool/tarantool/issues/4489#issuecomment-530448023 > > I'll cite my answer from this issue: > > | I would say that it should not allow more then a string: say, > | `'' .. box.NULL` gives an error, so > | `box.error.new(box.error.CFG, 'foo', 'bar') .. box.NULL` should too. > | At the first glance it looks that we should allow only an error as > | the first argument and a string, a number or an error as the second > | one. > | > | I think it is harmless, so I'm okay in general. > > Yep, the example with box.NULL works correct with your implementation. > But not with ffi.new('char'). Not even with {}. > > Also I was a bit out of context and don't know that a metamethod of a > right value can be invoked if there are no metamethod in a left one. So > 'we should allow only an error as the first argument' is not correct: > the first and the seconds arguments should be symmetric. > > To sum up I propose to follow logic of concatenation operation of a > string: it allows only certain data types to be concatenated with it: > strings and numbers. > > We maybe can reuse string's checks: > > | if lhs is an error: > | return tostring(lhs) .. rhs > | else if rhs is an error: > | return lhs .. tostring(rhs) > | else: > | return error('error_mt.__concat(): neither of args is an error') > > Here we handle everything as I see: nils, tables (with and without its > own __concat), numbers, cdata and userdata (again, with and without > __concat). > > Anyway, let's test an implementation more carefully. > > The only flaw I see that if a concatenation error occurs, then it will > state an error of concatenate a string with something (or something with > a string) rather then struct error. > > WBR, Alexander Turenko.