From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (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 5B2E4441841 for ; Thu, 26 Mar 2020 21:06:41 +0300 (MSK) Date: Thu, 26 Mar 2020 18:06:40 +0000 From: Nikita Pettik Message-ID: <20200326180640.GA6278@tarantool.org> References: <1816245c768216733779be5c7420cc60725287ea.1585097339.git.korablev@tarantool.org> <20200326165049.GC21134@atlas> <20200326175915.GC5718@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200326175915.GC5718@tarantool.org> 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 17:59, Nikita Pettik wrote: > 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). Oh, sorry, my bad. It is documented convention: tarantool.io/en/doc/1.10/dev_guide/c_style_guide/ > > 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