Re: [Tarantool-patches] [PATCH 3/7] box/error: don't set error created via box.error.new to diag

[Nikita Pettik <korablev@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.