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

["Timur Safin" <tsafin@tarantool.org>]

: From: Aleksandr Lyapunov
: Subject: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C
: and C++
: 
: We should not try-catch something that must not throw.
: It's dumb.
: 
: We should use function-try-block to convert C++ function to C.
: It's short, simple and does not create big intends.
: 
...
: @@ -2672,17 +2646,14 @@ on_replace_dd_index(struct trigger * /* trigger
: */, void *event)
:  		if (def == NULL)
:  			return -1;
:  		index_def_update_optionality(def, alter->new_min_field_count);
: -		try {
: -			if (def->opts.is_unique) {
: -				(void) new CreateConstraintID(
: -					alter, iid == 0 ? CONSTRAINT_TYPE_PK :
: -					CONSTRAINT_TYPE_UNIQUE, def->name);
: -			}
: -			(void) new CreateIndex(alter, def);
: -		} catch (Exception *e) {
: -			index_def_delete(def);
: -			return -1;
: +		auto guard = make_scoped_guard([=] { index_def_delete(def);
: });

I knew a lot of C++ code guidelines which specifically forbid 
capturing all arguments in C++ lambdas via copy or reference
and insist on explicit names passage. I do believe it makes some 
sense. What about to apply the same policy here and after? 


: +		if (def->opts.is_unique) {
: +			(void) new CreateConstraintID(
: +				alter, iid == 0 ? CONSTRAINT_TYPE_PK :
: +				CONSTRAINT_TYPE_UNIQUE, def->name);
:  		}
: +		(void) new CreateIndex(alter, def);
: +		guard.is_active = false;
:  	}
:  	/* Case 3 and 4: check if we need to rebuild index data. */
:  	if (old_index != NULL && new_tuple != NULL) {

Also, this operator new usage as wrapper around rlist operations 
looks ... not very much idiomatic, and unexpected at least (we do 
see new allocating memory which is instantly forgotten. WTF the 
immediate reaction). If we introduce here some sort of reference
counted allocations what about to wrap it somehow differently, 
say via templatized make_rlist_object<T> or something like that?
(even simple macros would looks clearer, IMVHO).

I'm not insisting on that approach, just saying...

Timur