From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp20.mail.ru (smtp20.mail.ru [94.100.179.251]) (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 D5FCD452566 for ; Mon, 11 Nov 2019 17:36:49 +0300 (MSK) Date: Mon, 11 Nov 2019 17:36:48 +0300 From: Sergey Ostanevich Message-ID: <20191111143648.GA10433@tarantool.org> References: <09dd946a8304f3865ada1638f610a4f9ba2fd3eb.1569253692.git.i.kosarev@tarantool.org> <20191030103454.GB35607@tarantool.org> <1572433344.407730155@f522.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1572433344.407730155@f522.i.mail.ru> Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 08/20] refactoring: remove exceptions from func_def_new_from_tuple List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@freelists.org, tarantool-patches Hi! No need to discuss - I believe your and Georgy argument is valid. Let it be like this. LGTM Sergos On 30 Oct 14:02, Ilya Kosarev wrote: > > Hi! > > Thanks for your review. > I guess it is debatable. On the one hand, it will be easier to understand which line did the error happened in, on the other hand, it looks better, when such blocks are combined. "Better layout" might seem minorish, however, if we need to debug this code, we can alway place a breakpoint and look at old_def & new_def intently to discover which one is actually NULL. Also Georgy recommended the opposite thing in his review for patch 5 ( https://www.freelists.org/post/tarantool-patches/PATCH-v4-0520-refactoring-clear-privilege-managing-triggers-from-exceptions,1 ), so i guess we can discuss it verbally. > Sincerely, > Ilya Kosarev > > > >Среда, 30 октября 2019, 13:34 +03:00 от Sergey Ostanevich : > > > >On 23 Sep 18:56, Ilya Kosarev wrote: > > > >Hi! > > > >Just one nit below, LGTM with follow-up. > > > >Sergos > > > > > >> func_def_new_from_tuple is used in on_replace_dd_func therefore > >> it has to be cleared from exceptions. Now it doesn't throw any > >> more. It means we also need to clear from exceptions it's > >> subsidiary function func_def_get_ids_from_tuple. > >> Their usages are updated. > >> > >> Part of #4247 > >> --- > >> src/box/alter.cc | 149 ++++++++++++++++++++++++++++++----------------- > >> 1 file changed, 97 insertions(+), 52 deletions(-) > >> > >> diff --git a/src/box/alter.cc b/src/box/alter.cc > >> index 10a5f8d64..3d1c59189 100644 > >> --- a/src/box/alter.cc > >> +++ b/src/box/alter.cc > >> @@ -3132,6 +3175,8 @@ on_replace_dd_func(struct trigger * /* trigger */, void *event) > >> }); > >> old_def = func_def_new_from_tuple(old_tuple); > >Shall we return error here, rather than possibly mask it with another one at > >the next line? > > > >> new_def = func_def_new_from_tuple(new_tuple); > >> + if (old_def == NULL || new_def == NULL) > >> + return -1; > >> if (func_def_cmp(new_def, old_def) != 0) { > >> diag_set(ClientError, ER_UNSUPPORTED, "function", > >> "alter"); > >> -- > >> 2.17.1 > >> > >> > > > -- > Ilya Kosarev