From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tsafin@tarantool.org>
Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58])
 (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 90BA2445320
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  8 Jul 2020 13:41:35 +0300 (MSK)
From: "Timur Safin" <tsafin@tarantool.org>
References: <1594199230-26036-1-git-send-email-alyapunov@tarantool.org>
 <1594199230-26036-3-git-send-email-alyapunov@tarantool.org>
In-Reply-To: <1594199230-26036-3-git-send-email-alyapunov@tarantool.org>
Date: Wed, 8 Jul 2020 13:41:34 +0300
Message-ID: <022601d65514$59d7a560$0d86f020$@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: 7bit
Content-Language: ru
Subject: Re: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry
	C	and C++
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: 'Aleksandr Lyapunov' <alyapunov@tarantool.org>, tarantool-patches@dev.tarantool.org
Cc: v.shpilevoy@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