From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154])
 (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 1F82D4696C3
 for <tarantool-patches@dev.tarantool.org>;
 Wed, 25 Mar 2020 04:02:32 +0300 (MSK)
Date: Wed, 25 Mar 2020 01:02:31 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200325010231.GA30598@tarantool.org>
References: <cover.1582119629.git.korablev@tarantool.org>
 <f73a37259ffbe271e9353361f4e04c3190ee9a26.1582119629.git.korablev@tarantool.org>
 <c2b0e527-436f-876d-573e-3f35add455d6@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <c2b0e527-436f-876d-573e-3f35add455d6@tarantool.org>
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 <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

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.
 
> > +		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.