From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 63C304696C3 for ; Thu, 16 Apr 2020 03:02:20 +0300 (MSK) References: <2bb678a8aed7e1533ca2c626048080c4c1c5520b.1586505741.git.lvasiliev@tarantool.org> <5688aaca-1326-aed0-9dbc-a13e616539fe@tarantool.org> <40152977-6032-60e4-0396-f773d3dd395b@tarantool.org> From: Vladislav Shpilevoy Message-ID: <2b4b8329-437a-429e-577b-8e1bb49d14c1@tarantool.org> Date: Thu, 16 Apr 2020 02:02:18 +0200 MIME-Version: 1.0 In-Reply-To: <40152977-6032-60e4-0396-f773d3dd395b@tarantool.org> Content-Type: text/plain; charset="utf-8" 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: lvasiliev Cc: tarantool-patches@dev.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.