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

[lvasiliev <lvasiliev@tarantool.org>]

Hi! Thanks you for the feedback.

On 16.04.2020 3:02, Vladislav Shpilevoy wrote:
> 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.
Hmm, ok.
> 
>>>> + */
>>>> +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.
Ok. I will replace it in the new version of the patchset.
> 
>>>
>>>>     */
>>>>    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.
> 
No, from user's point of view it have only type without any codes (In 
accordance with the task). Besides, I think it's strange to have an 
AccessDeniedError with code "ER_SQL_PREPARE" (other branch of 
hierarchy). And user doesn't have an access to "code" of error.