Re: [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes!

>> 6. Make_type() perfectly fits in one line.
>>
>>> +
>>> +CustomError::CustomError(const char *file, unsigned int line,
>>> +             const char *custom_type)
>>> +    :ClientError(&type_CustomError, file, line, ER_CUSTOM_ERROR)
>>> +{
>>> +    error_format_msg(this, tnt_errcode_desc(m_errcode),
>>> +             custom_type ?: "");
>>
>> 7. custom_type is never NULL.
> Why? It's a constructor argument.

And still it is never NULL. I couldn't find any place, where
you passed NULL here, and anyway that would be incorrect. Type
can't be just NULL. How to present it in Lua then? nil? Doubt
it.

>>> + */
>>> +const char *
>>> +box_custom_error_type(const box_error_t *e);
>>> +
>>> +/**
>>>    * Add error to the diagnostic area. In contrast to box_error_set()
>>>    * it does not replace previous error being set, but rather link
>>>    * them into list.
>>> @@ -155,15 +166,24 @@ box_error_add(const char *file, unsigned line, uint32_t code,
>>>     /**
>>>    * Construct error object without setting it in the diagnostics
>>> - * area. On the memory allocation fail returns NULL.
>>> + * area. On the memory allocation fail returns  OutOfMemory error.
>>
>> 10. Unneccessary diff.
> The comment is wrong. How I can fix it?

Yeah, I just realized that after you said box_error_new() never
returns NULL. Then probably it is ok to change it.

>>
>>>    */
>>>   struct error *
>>>   box_error_new(const char *file, unsigned line, uint32_t code,
>>>             const char *fmt, ...);
>>>   +/**
>>> + * Construct error object without setting it in the diagnostics
>>> + * area. On the memory allocation fail returns OutOfMemory error.
>>
>> 11. This is a copy-paste box box_error_new() comment. Better write
>> here a couple of words how is it different. Or better merge box_error_new()
>> and box_custom_error_new(). Because in Lua I can specify both code
>> and type. Don't see why can't I do this in C.
> I thought we can’t change public API.

This is not public API. If something is not compiled into module.h,
it is not public (in C. For Lua rules are different).

>>> @@ -111,9 +148,16 @@ raise:
>>>           line = info.currentline;
>>>       }
>>>   -    struct error *err = box_error_new(file, line, code, "%s", reason);
>>> -    if (tb_parsed)
>>> -        err->traceback_mode = tb_mode;
>>> +    struct error *err = NULL;
>>> +    if (args.custom) {
>>> +        err = box_custom_error_new(file, line, args.custom,
>>> +                       "%s", args.reason);
>>
>> 18. Code argument is ignored here. Although I am not sure it shouldn't
>> be ignored. From what I remember, we decided that code should stay,
>> but I may be wrong.
> CustomError type has a predefinded code. Why it shouldn't be ignored?

Because it is not a 'ClientError' object from user's point of view. The
fact that we implemented it as a descendant of 'ClientError' class should
not leak into the API. User can't assume anything about error hierarchies.
He does not even see them.

From user's point of view there are separate built-in 'ClientError'. And
separate 'CustomError'. Both accept error code now. But 'CustomError'
ignores it somewhy. Does not look right.