From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 CC9D04696C3 for ; Thu, 26 Mar 2020 03:22:53 +0300 (MSK) References: <20200325010231.GA30598@tarantool.org> From: Vladislav Shpilevoy Message-ID: <32acd313-8d54-5496-55e2-0613fe4bd883@tarantool.org> Date: Thu, 26 Mar 2020 01:22:52 +0100 MIME-Version: 1.0 In-Reply-To: <20200325010231.GA30598@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 3/7] 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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org On 25/03/2020 02:02, Nikita Pettik wrote: > On 22 Feb 18:18, Vladislav Shpilevoy wrote: >> Thanks for the patch! >> >>> diff --git a/src/box/error.cc b/src/box/error.cc >>> index 7dfe1b3ee..824a4617c 100644 >>> --- a/src/box/error.cc >>> +++ b/src/box/error.cc >>> @@ -86,6 +86,22 @@ box_error_set(const char *file, unsigned line, uint32_t code, >>> return -1; >>> } >>> >>> +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; >> >> 4. You can now make box_error_set() call box_error_construct() + >> diag_set(), to avoid code duplication. > > I don't think it is worth it since I have to introduce one more > 'proxy' function which accepts va_list arguments. Skipped. Yeah, I forgot that it is not as as trivial to forward ... as in Lua. >>> + va_list ap; >>> + va_start(ap, fmt); >>> + error_vformat_msg(e, fmt, ap); >>> + va_end(ap); >>> + } >>> + return e; >>> +} >>> + >>> /* }}} */ >>> >>> struct rmean *rmean_error = NULL; >>> diff --git a/src/box/error.h b/src/box/error.h >>> index b8c7cf73d..42043ef80 100644 >>> --- a/src/box/error.h >>> +++ b/src/box/error.h >>> @@ -129,6 +129,14 @@ int >>> box_error_set(const char *file, unsigned line, uint32_t code, >>> const char *format, ...); >>> >>> +/** >>> + * Construct error object without setting it in the diagnostics >>> + * area. On the memory allocation fail returns NULL. >>> + */ >>> +struct error * >>> +box_error_construct(const char *file, unsigned line, uint32_t code, >>> + const char *fmt, ...); >> >> 5. You added the method to the public C API (it is inside 'cond public' >> comments). I don't think we should do that. At least until someone >> asked so. >> >> Also why not box_error_new()? 'Construct' seems to be a very strange name. > > IMHO _new 'suffix' implies allocating new object, meanwhile _construct - > filling in already allocated object. It was my understanding. Exactly. And box_error_construct() allocates a new object. You even said that in the comment. So where am I wrong? Btw, for initialization of an object in-place we use '_create'. '_init' is used for initialization of a module. But '_construct' - never.