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

[Vladislav Shpilevoy <v.shpilevoy@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.