[Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type
lvasiliev
lvasiliev at tarantool.org
Thu Apr 16 12:18:51 MSK 2020
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.
More information about the Tarantool-patches
mailing list