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 9548F445320 for ; Sat, 11 Jul 2020 22:53:24 +0300 (MSK) References: <1594199230-26036-1-git-send-email-alyapunov@tarantool.org> <1594199230-26036-3-git-send-email-alyapunov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <84562bbc-23eb-8f00-3077-57719fb9f81e@tarantool.org> Date: Sat, 11 Jul 2020 21:53:21 +0200 MIME-Version: 1.0 In-Reply-To: <1594199230-26036-3-git-send-email-alyapunov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org Thanks for the patch! On 08/07/2020 11:07, Aleksandr Lyapunov wrote: > We should not try-catch something that must not throw. > It's dumb. Yeah, well. It is not so. It was done for a certain reason - expecting future conversion to C, to reduce diff. Because when converted to C, this code will check individual places, not entire function like SQLite does or C++ with exceptions. Moreover, even if we will keep C++, we will never use exceptions more than now for sure. So the error checking will stay in C way as I expect. I don't know about other things Timur told about how to capture things in lambda, and how is it forbidden, but I do know that the exceptions are often forbidden, and for reasons. So I don't really think it is a good idea to extend the code throwing exceptions. Except a few places which actually didn't throw anything ever, these ones you are right to fix. For example, on_commit and on_rollback triggers - they never throw indeed. They can panic, at most. Much better solution would be to eliminate exceptions from there. Not mask them even more. You wouldn't need try-catch and noexcept at all. See 2 comments below. > We should use function-try-block to convert C++ function to C. > It's short, simple and does not create big intends. > > Closes #5153 > --- > src/box/alter.cc | 234 ++++++++++++++++++++----------------------------------- > 1 file changed, 85 insertions(+), 149 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 1a7949e..9a0192f 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1025,12 +1025,8 @@ alter_space_rollback(struct trigger *trigger, void * /* event */) noexcept > struct alter_space *alter = (struct alter_space *) trigger->data; > /* Rollback alter ops */ > class AlterSpaceOp *op; > - try { > - rlist_foreach_entry(op, &alter->ops, link) { > - op->rollback(alter); > - } > - } catch (Exception *e) { > - return -1; > + rlist_foreach_entry(op, &alter->ops, link) { > + op->rollback(alter); > } 1. This one is good. Try-catch wasn't needed here. But it looks like related to the previous commit. Not to this one. > /* Rebuild index maps once for all indexes. */ > space_fill_index_map(alter->old_space); > @@ -1965,7 +1961,7 @@ on_create_space_rollback(struct trigger *trigger, void *event) > */ > int > alter_space_move_indexes(struct alter_space *alter, uint32_t begin, > - uint32_t end) > + uint32_t end) try 2. This one I don't like. It makes harder to eliminate exceptions in future. Because will need to check each function call in this function to see if it throws. Before your patch we could locate such places by simple grep by 'try' and 'catch'. However it is arguable when it comes to constructors. Anyway we will change them and won't miss these places. But when it comes to function calls, it is not good. For example, when you move alter_space_do() out of explicit try-catch.