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 31C4B46970E for ; Fri, 10 Jan 2020 15:32:03 +0300 (MSK) Date: Fri, 10 Jan 2020 15:32:06 +0300 From: Alexander Turenko Message-ID: <20200110123205.fu5kg376hxcq2n2f@tkn_work_nb> References: <20191111145009.91425-1-olegrok@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191111145009.91425-1-olegrok@tarantool.org> 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: Oleg Babin Cc: Oleg Babin , tarantool-patches@dev.tarantool.org 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.