Re: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++

[Vladislav Shpilevoy <v.shpilevoy@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.