[Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++
Timur Safin
tsafin at tarantool.org
Wed Jul 8 13:41:34 MSK 2020
: 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
More information about the Tarantool-patches
mailing list