From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp10.mail.ru (smtp10.mail.ru [94.100.181.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id AF8BA4696C3 for ; Thu, 16 Apr 2020 12:18:52 +0300 (MSK) References: <2bb678a8aed7e1533ca2c626048080c4c1c5520b.1586505741.git.lvasiliev@tarantool.org> <5688aaca-1326-aed0-9dbc-a13e616539fe@tarantool.org> <40152977-6032-60e4-0396-f773d3dd395b@tarantool.org> <2b4b8329-437a-429e-577b-8e1bb49d14c1@tarantool.org> From: lvasiliev Message-ID: <428aeafc-58b5-899d-b62e-bf08853d3366@tarantool.org> Date: Thu, 16 Apr 2020 12:18:51 +0300 MIME-Version: 1.0 In-Reply-To: <2b4b8329-437a-429e-577b-8e1bb49d14c1@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH v2 2/5] error: Add the custom error type List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.