From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 30C4E441841 for ; Thu, 26 Mar 2020 20:59:16 +0300 (MSK) Date: Thu, 26 Mar 2020 17:59:15 +0000 From: Nikita Pettik Message-ID: <20200326175915.GC5718@tarantool.org> References: <1816245c768216733779be5c7420cc60725287ea.1585097339.git.korablev@tarantool.org> <20200326165049.GC21134@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200326165049.GC21134@atlas> Subject: Re: [Tarantool-patches] [PATCH v2 05/10] box/error: don't set error created via box.error.new to diag List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Konstantin Osipov , tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org On 26 Mar 19:50, Konstantin Osipov wrote: > * Nikita Pettik [20/03/25 09:32]: > > To achieve this let's refactor luaT_error_create() to return error > > object instead of setting it via box_error_set(). > > luaT_error_create() is used both to handle box.error() and > > box.error.new() invocations, and box.error() is still expected to set > > error to diagnostic area. So, luaT_error_call() which implements > > box.error() processing at the end calls diag_set_error(). > > It is worth mentioning that net.box module relied on the fact that > > box.error.new() set error to diagnostic area: otherwise request errors > > don't get to diagnostic area on client side. > > > > I know there was a discussion about construct vs create vs new. It was not really discussion, I accidentally didn't notice that box_error_construct() in fact allocated memory (meanwhile I thought it didn't). I've already renamed it to box_error_new(). > Let me remind about approved name pairs: > > obj_create/obj_destroy - constructor/destructor, without > allocation/deallocation. > > obj_new/obj_delete - constructor/destructor with > allocation/deallocation > > init/free - library initialization/destruction > > _construct is not in the vocabulary. I'd avoid it. Ok, Vlad already guided me (but it would be nice to see this manual in our dev docs). > I know this is bikeshed, pls feel free to ignore in scope of this > patch. And sorry for jumping in late. > > > +struct error * > > +box_error_construct(const char *file, unsigned line, uint32_t code, > > + const char *fmt, ...) > > +{ > > + struct error *e = BuildClientError(file, line, ER_UNKNOWN); > > + ClientError *client_error = type_cast(ClientError, e); > > + if (client_error != NULL) { > > + client_error->m_errcode = code; > > + va_list ap; > > + va_start(ap, fmt); > > + error_vformat_msg(e, fmt, ap); > > + va_end(ap); > > + } > > + return e; > > +} > > if box_error_new() is already taken by the public api, options are > box_client_error_new() or box_build_client_error()? No, box_error_new() name is still available. I've renamed box_error_construct() to box_error_new() and pushed branch containing this changes. > > -- > Konstantin Osipov, Moscow, Russia