* [Tarantool-patches] [PATCH 0/2] Simplify alter.cc @ 2020-07-08 8:43 Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 1/2] alter: use good c++ style Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 0 siblings, 2 replies; 6+ messages in thread From: Aleksandr Lyapunov @ 2020-07-08 8:43 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Since we use C++ we need to use in a way that benefits its advantages. As I see, there are tons of try-catch blocks in alter.cc. We should use more modern techniques and approaches. Aleksandr Lyapunov (2): alter: use good c++ style alter: use proper way to marry C and C++ src/box/alter.cc | 418 +++++++++++++++++++++++-------------------------------- 1 file changed, 177 insertions(+), 241 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH 1/2] alter: use good c++ style 2020-07-08 8:43 [Tarantool-patches] [PATCH 0/2] Simplify alter.cc Aleksandr Lyapunov @ 2020-07-08 8:43 ` Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 1 sibling, 0 replies; 6+ messages in thread From: Aleksandr Lyapunov @ 2020-07-08 8:43 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy We should use noecept where it's appropriate. 1.It informs a developer he could not care about exception safety 2.It instructs a compiler to check throws if possible We should override for compile-time check of misprints in names of methods and types of arguments. Part of #5153 --- src/box/alter.cc | 184 +++++++++++++++++++++++++++---------------------------- 1 file changed, 92 insertions(+), 92 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index bb42548..1a7949e 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -760,7 +760,7 @@ struct alter_space; class AlterSpaceOp { public: - AlterSpaceOp(struct alter_space *alter); + explicit AlterSpaceOp(struct alter_space *alter); /** Link in alter_space::ops. */ struct rlist link; @@ -769,7 +769,7 @@ public: * the space definition and/or key list that will be used * for creating the new space. Must not yield or fail. */ - virtual void alter_def(struct alter_space * /* alter */) {} + virtual void alter_def(struct alter_space * /* alter */) noexcept {} /** * Called after creating a new space. Used for performing * long-lasting operations, such as index rebuild or format @@ -783,21 +783,21 @@ public: * state to the new space (e.g. move unchanged indexes). * Must not yield or fail. */ - virtual void alter(struct alter_space * /* alter */) {} + virtual void alter(struct alter_space * /* alter */) noexcept {} /** * Called after the change has been successfully written * to WAL. Must not fail. */ virtual void commit(struct alter_space * /* alter */, - int64_t /* signature */) {} + int64_t /* signature */) noexcept {} /** * Called in case a WAL error occurred. It is supposed to undo * the effect of AlterSpaceOp::prepare and AlterSpaceOp::alter. * Must not fail. */ - virtual void rollback(struct alter_space * /* alter */) {} + virtual void rollback(struct alter_space * /* alter */) noexcept {} - virtual ~AlterSpaceOp() {} + virtual ~AlterSpaceOp() noexcept {} void *operator new(size_t size) { @@ -979,7 +979,7 @@ struct mh_i32_t *AlterSpaceLock::registry; * Replace the old space with a new one in the space cache. */ static int -alter_space_commit(struct trigger *trigger, void *event) +alter_space_commit(struct trigger *trigger, void *event) noexcept { struct txn *txn = (struct txn *) event; struct alter_space *alter = (struct alter_space *) trigger->data; @@ -997,12 +997,8 @@ alter_space_commit(struct trigger *trigger, void *event) * indexes into their new places. */ class AlterSpaceOp *op; - try { - rlist_foreach_entry(op, &alter->ops, link) - op->commit(alter, signature); - } catch (Exception *e) { - return -1; - } + rlist_foreach_entry(op, &alter->ops, link) + op->commit(alter, signature); alter->new_space = NULL; /* for alter_space_delete(). */ /* @@ -1024,7 +1020,7 @@ alter_space_commit(struct trigger *trigger, void *event) * alter_space_commit() failure (unlikely) */ static int -alter_space_rollback(struct trigger *trigger, void * /* event */) +alter_space_rollback(struct trigger *trigger, void * /* event */) noexcept { struct alter_space *alter = (struct alter_space *) trigger->data; /* Rollback alter ops */ @@ -1183,9 +1179,9 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space *alter) class CheckSpaceFormat: public AlterSpaceOp { public: - CheckSpaceFormat(struct alter_space *alter) + explicit CheckSpaceFormat(struct alter_space *alter) :AlterSpaceOp(alter) {} - virtual void prepare(struct alter_space *alter); + void prepare(struct alter_space *alter) override; }; void @@ -1217,15 +1213,15 @@ public: * names into an old dictionary and deletes new one. */ struct tuple_dictionary *new_dict; - virtual void alter_def(struct alter_space *alter); - virtual void alter(struct alter_space *alter); - virtual void rollback(struct alter_space *alter); - virtual ~ModifySpace(); + void alter_def(struct alter_space *alter) noexcept override; + void alter(struct alter_space *alter) noexcept override; + void rollback(struct alter_space *alter) noexcept override; + ~ModifySpace() noexcept override; }; /** Amend the definition of the new space. */ void -ModifySpace::alter_def(struct alter_space *alter) +ModifySpace::alter_def(struct alter_space *alter) noexcept { /* * Use the old dictionary for the new space, because @@ -1244,7 +1240,7 @@ ModifySpace::alter_def(struct alter_space *alter) } void -ModifySpace::alter(struct alter_space *alter) +ModifySpace::alter(struct alter_space *alter) noexcept { /* * Move new names into an old dictionary, which already is @@ -1255,12 +1251,12 @@ ModifySpace::alter(struct alter_space *alter) } void -ModifySpace::rollback(struct alter_space *alter) +ModifySpace::rollback(struct alter_space *alter) noexcept { tuple_dictionary_swap(alter->new_space->def->dict, new_dict); } -ModifySpace::~ModifySpace() +ModifySpace::~ModifySpace() noexcept { if (new_dict != NULL) tuple_dictionary_unref(new_dict); @@ -1276,9 +1272,9 @@ public: DropIndex(struct alter_space *alter, struct index *index) :AlterSpaceOp(alter), old_index(index) {} struct index *old_index; - virtual void alter_def(struct alter_space *alter); - virtual void prepare(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t lsn); + void alter_def(struct alter_space *alter) noexcept override; + void prepare(struct alter_space *alter) override; + void commit(struct alter_space *alter, int64_t lsn) noexcept override; }; /* @@ -1286,7 +1282,7 @@ public: * the new index from it. */ void -DropIndex::alter_def(struct alter_space * /* alter */) +DropIndex::alter_def(struct alter_space * /* alter */) noexcept { rlist_del_entry(old_index->def, link); } @@ -1300,7 +1296,7 @@ DropIndex::prepare(struct alter_space *alter) } void -DropIndex::commit(struct alter_space *alter, int64_t signature) +DropIndex::commit(struct alter_space *alter, int64_t signature) noexcept { (void)alter; index_commit_drop(old_index, signature); @@ -1318,18 +1314,18 @@ public: :AlterSpaceOp(alter), iid(iid_arg) {} /** id of the index on the move. */ uint32_t iid; - virtual void alter(struct alter_space *alter); - virtual void rollback(struct alter_space *alter); + void alter(struct alter_space *alter) noexcept override; + void rollback(struct alter_space *alter) noexcept override; }; void -MoveIndex::alter(struct alter_space *alter) +MoveIndex::alter(struct alter_space *alter) noexcept { space_swap_index(alter->old_space, alter->new_space, iid, iid); } void -MoveIndex::rollback(struct alter_space *alter) +MoveIndex::rollback(struct alter_space *alter) noexcept { space_swap_index(alter->old_space, alter->new_space, iid, iid); } @@ -1360,23 +1356,23 @@ public: struct index *old_index; struct index *new_index; struct index_def *new_index_def; - virtual void alter_def(struct alter_space *alter); - virtual void alter(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t lsn); - virtual void rollback(struct alter_space *alter); - virtual ~ModifyIndex(); + void alter_def(struct alter_space *alter) noexcept override; + void alter(struct alter_space *alter) noexcept override; + void commit(struct alter_space *alter, int64_t lsn) noexcept override; + void rollback(struct alter_space *alter) noexcept override; + ~ModifyIndex() noexcept override; }; /** Update the definition of the new space */ void -ModifyIndex::alter_def(struct alter_space *alter) +ModifyIndex::alter_def(struct alter_space *alter) noexcept { rlist_del_entry(old_index->def, link); index_def_list_add(&alter->key_list, new_index_def); } void -ModifyIndex::alter(struct alter_space *alter) +ModifyIndex::alter(struct alter_space *alter) noexcept { new_index = space_index(alter->new_space, new_index_def->iid); assert(old_index->def->iid == new_index->def->iid); @@ -1392,14 +1388,14 @@ ModifyIndex::alter(struct alter_space *alter) } void -ModifyIndex::commit(struct alter_space *alter, int64_t signature) +ModifyIndex::commit(struct alter_space *alter, int64_t signature) noexcept { (void)alter; index_commit_modify(new_index, signature); } void -ModifyIndex::rollback(struct alter_space *alter) +ModifyIndex::rollback(struct alter_space *alter) noexcept { /* * Restore indexes. @@ -1411,7 +1407,7 @@ ModifyIndex::rollback(struct alter_space *alter) index_update_def(old_index); } -ModifyIndex::~ModifyIndex() +ModifyIndex::~ModifyIndex() noexcept { index_def_delete(new_index_def); } @@ -1427,15 +1423,15 @@ public: CreateIndex(struct alter_space *alter, struct index_def *def) :AlterSpaceOp(alter), new_index(NULL), new_index_def(def) {} - virtual void alter_def(struct alter_space *alter); - virtual void prepare(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t lsn); - virtual ~CreateIndex(); + void alter_def(struct alter_space *alter) noexcept override; + void prepare(struct alter_space *alter) override; + void commit(struct alter_space *alter, int64_t lsn) noexcept override; + ~CreateIndex() noexcept override; }; /** Add definition of the new key to the new space def. */ void -CreateIndex::alter_def(struct alter_space *alter) +CreateIndex::alter_def(struct alter_space *alter) noexcept { index_def_list_add(&alter->key_list, new_index_def); } @@ -1475,7 +1471,7 @@ CreateIndex::prepare(struct alter_space *alter) } void -CreateIndex::commit(struct alter_space *alter, int64_t signature) +CreateIndex::commit(struct alter_space *alter, int64_t signature) noexcept { (void) alter; assert(new_index != NULL); @@ -1483,7 +1479,7 @@ CreateIndex::commit(struct alter_space *alter, int64_t signature) new_index = NULL; } -CreateIndex::~CreateIndex() +CreateIndex::~CreateIndex() noexcept { if (new_index != NULL) index_abort_create(new_index); @@ -1516,15 +1512,15 @@ public: struct index_def *new_index_def; /** Old index index_def. */ struct index_def *old_index_def; - virtual void alter_def(struct alter_space *alter); - virtual void prepare(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t signature); - virtual ~RebuildIndex(); + void alter_def(struct alter_space *alter) noexcept override; + void prepare(struct alter_space *alter) override; + void commit(struct alter_space *alter, int64_t signature) noexcept override; + ~RebuildIndex() noexcept override; }; /** Add definition of the new key to the new space def. */ void -RebuildIndex::alter_def(struct alter_space *alter) +RebuildIndex::alter_def(struct alter_space *alter) noexcept { rlist_del_entry(old_index_def, link); index_def_list_add(&alter->key_list, new_index_def); @@ -1540,7 +1536,7 @@ RebuildIndex::prepare(struct alter_space *alter) } void -RebuildIndex::commit(struct alter_space *alter, int64_t signature) +RebuildIndex::commit(struct alter_space *alter, int64_t signature) noexcept { struct index *old_index = space_index(alter->old_space, old_index_def->iid); @@ -1551,7 +1547,7 @@ RebuildIndex::commit(struct alter_space *alter, int64_t signature) new_index = NULL; } -RebuildIndex::~RebuildIndex() +RebuildIndex::~RebuildIndex() noexcept { if (new_index != NULL) index_abort_create(new_index); @@ -1595,9 +1591,10 @@ public: TruncateIndex(struct alter_space *alter, uint32_t iid) : AlterSpaceOp(alter), iid(iid), old_index(NULL), new_index(NULL) {} - virtual void prepare(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t signature); - virtual ~TruncateIndex(); + void prepare(struct alter_space *alter) override; + void commit(struct alter_space *alter, + int64_t signature) noexcept override; + ~TruncateIndex() override; }; void @@ -1627,7 +1624,7 @@ TruncateIndex::prepare(struct alter_space *alter) } void -TruncateIndex::commit(struct alter_space *alter, int64_t signature) +TruncateIndex::commit(struct alter_space *alter, int64_t signature) noexcept { (void)alter; index_commit_drop(old_index, signature); @@ -1635,7 +1632,7 @@ TruncateIndex::commit(struct alter_space *alter, int64_t signature) new_index = NULL; } -TruncateIndex::~TruncateIndex() +TruncateIndex::~TruncateIndex() noexcept { if (new_index == NULL) return; @@ -1652,14 +1649,14 @@ class UpdateSchemaVersion: public AlterSpaceOp public: UpdateSchemaVersion(struct alter_space * alter) :AlterSpaceOp(alter) {} - virtual void alter(struct alter_space *alter); + void alter(struct alter_space *alter) noexcept override; }; void -UpdateSchemaVersion::alter(struct alter_space *alter) +UpdateSchemaVersion::alter(struct alter_space *alter) noexcept { - (void)alter; - ++schema_version; + (void) alter; + ++schema_version; } /** @@ -1679,10 +1676,10 @@ public: RebuildCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter), ck_constraint(RLIST_HEAD_INITIALIZER(ck_constraint)) {} struct rlist ck_constraint; - virtual void prepare(struct alter_space *alter); - virtual void alter(struct alter_space *alter); - virtual void rollback(struct alter_space *alter); - virtual ~RebuildCkConstraints(); + void prepare(struct alter_space *alter) override; + void alter(struct alter_space *alter) noexcept override; + void rollback(struct alter_space *alter) noexcept override; + ~RebuildCkConstraints() override; }; void @@ -1711,18 +1708,18 @@ RebuildCkConstraints::space_swap_ck_constraint(struct space *old_space, } void -RebuildCkConstraints::alter(struct alter_space *alter) +RebuildCkConstraints::alter(struct alter_space *alter) noexcept { space_swap_ck_constraint(alter->old_space, alter->new_space); } void -RebuildCkConstraints::rollback(struct alter_space *alter) +RebuildCkConstraints::rollback(struct alter_space *alter) noexcept { space_swap_ck_constraint(alter->new_space, alter->old_space); } -RebuildCkConstraints::~RebuildCkConstraints() +RebuildCkConstraints::~RebuildCkConstraints() noexcept { struct ck_constraint *old_ck_constraint, *tmp; rlist_foreach_entry_safe(old_ck_constraint, &ck_constraint, link, tmp) { @@ -1749,8 +1746,8 @@ class MoveCkConstraints: public AlterSpaceOp struct space *new_space); public: MoveCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {} - virtual void alter(struct alter_space *alter); - virtual void rollback(struct alter_space *alter); + void alter(struct alter_space *alter) noexcept override; + void rollback(struct alter_space *alter) noexcept override; }; void @@ -1764,13 +1761,13 @@ MoveCkConstraints::space_swap_ck_constraint(struct space *old_space, } void -MoveCkConstraints::alter(struct alter_space *alter) +MoveCkConstraints::alter(struct alter_space *alter) noexcept { space_swap_ck_constraint(alter->old_space, alter->new_space); } void -MoveCkConstraints::rollback(struct alter_space *alter) +MoveCkConstraints::rollback(struct alter_space *alter) noexcept { space_swap_ck_constraint(alter->new_space, alter->old_space); } @@ -1829,11 +1826,12 @@ public: if (new_id == NULL) diag_raise(); } - virtual void prepare(struct alter_space *alter); - virtual void alter(struct alter_space *alter); - virtual void rollback(struct alter_space *alter); - virtual void commit(struct alter_space *alter, int64_t signature); - virtual ~CreateConstraintID(); + void prepare(struct alter_space *alter) override; + void alter(struct alter_space *alter) noexcept override; + void rollback(struct alter_space *alter) noexcept override; + void commit(struct alter_space *alter, + int64_t signature) noexcept override; + ~CreateConstraintID() noexcept override; }; void @@ -1845,7 +1843,7 @@ CreateConstraintID::prepare(struct alter_space *alter) } void -CreateConstraintID::alter(struct alter_space *alter) +CreateConstraintID::alter(struct alter_space *alter) noexcept { /* Alter() can't fail, so can't just throw an error. */ if (space_add_constraint_id(alter->old_space, new_id) != 0) @@ -1853,14 +1851,15 @@ CreateConstraintID::alter(struct alter_space *alter) } void -CreateConstraintID::rollback(struct alter_space *alter) +CreateConstraintID::rollback(struct alter_space *alter) noexcept { space_delete_constraint_id(alter->new_space, new_id->name); new_id = NULL; } void -CreateConstraintID::commit(struct alter_space *alter, int64_t signature) +CreateConstraintID::commit(struct alter_space *alter, + int64_t signature) noexcept { (void) alter; (void) signature; @@ -1871,7 +1870,7 @@ CreateConstraintID::commit(struct alter_space *alter, int64_t signature) new_id = NULL; } -CreateConstraintID::~CreateConstraintID() +CreateConstraintID::~CreateConstraintID() noexcept { if (new_id != NULL) constraint_id_delete(new_id); @@ -1886,19 +1885,20 @@ public: DropConstraintID(struct alter_space *alter, const char *name) :AlterSpaceOp(alter), old_id(NULL), name(name) {} - virtual void alter(struct alter_space *alter); - virtual void commit(struct alter_space *alter , int64_t signature); - virtual void rollback(struct alter_space *alter); + void alter(struct alter_space *alter) noexcept override; + void commit(struct alter_space *alter, + int64_t signature) noexcept override; + void rollback(struct alter_space *alter) noexcept override; }; void -DropConstraintID::alter(struct alter_space *alter) +DropConstraintID::alter(struct alter_space *alter) noexcept { old_id = space_pop_constraint_id(alter->old_space, name); } void -DropConstraintID::commit(struct alter_space *alter, int64_t signature) +DropConstraintID::commit(struct alter_space *alter, int64_t signature) noexcept { (void) alter; (void) signature; @@ -1906,7 +1906,7 @@ DropConstraintID::commit(struct alter_space *alter, int64_t signature) } void -DropConstraintID::rollback(struct alter_space *alter) +DropConstraintID::rollback(struct alter_space *alter) noexcept { if (space_add_constraint_id(alter->new_space, old_id) != 0) { panic("Can't recover after constraint drop rollback (out of " -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ 2020-07-08 8:43 [Tarantool-patches] [PATCH 0/2] Simplify alter.cc Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 1/2] alter: use good c++ style Aleksandr Lyapunov @ 2020-07-08 8:43 ` Aleksandr Lyapunov 1 sibling, 0 replies; 6+ messages in thread From: Aleksandr Lyapunov @ 2020-07-08 8:43 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy 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. 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); } /* 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 { struct space *old_space = alter->old_space; bool is_min_field_count_changed; @@ -1988,17 +1984,9 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, new_def = index_def_dup(old_def); index_def_update_optionality(new_def, min_field_count); - try { - (void) new ModifyIndex(alter, old_index, new_def); - } catch (Exception *e) { - return -1; - } + (void) new ModifyIndex(alter, old_index, new_def); } else { - try { - (void) new MoveIndex(alter, old_def->iid); - } catch (Exception *e) { - return -1; - } + (void) new MoveIndex(alter, old_def->iid); } continue; } @@ -2013,20 +2001,14 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, index_def_update_optionality(new_def, min_field_count); auto guard = make_scoped_guard([=] { index_def_delete(new_def); }); if (!index_def_change_requires_rebuild(old_index, new_def)) - try { - (void) new ModifyIndex(alter, old_index, new_def); - } catch (Exception *e) { - return -1; - } + (void) new ModifyIndex(alter, old_index, new_def); else - try { - (void) new RebuildIndex(alter, new_def, old_def); - } catch (Exception *e) { - return -1; - } + (void) new RebuildIndex(alter, new_def, old_def); guard.is_active = false; } return 0; +} catch (Exception *) { + return -1; } /** @@ -2189,7 +2171,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event) * clumsy, so it is simply not done. */ static int -on_replace_dd_space(struct trigger * /* trigger */, void *event) +on_replace_dd_space(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2469,28 +2451,23 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) old_space->index_count, def->fields, def->field_count); - try { - (void) new CheckSpaceFormat(alter); - (void) new ModifySpace(alter, def); - (void) new RebuildCkConstraints(alter); - } catch (Exception *e) { - return -1; - } + (void) new CheckSpaceFormat(alter); + (void) new ModifySpace(alter, def); + (void) new RebuildCkConstraints(alter); def_guard.is_active = false; /* Create MoveIndex ops for all space indexes. */ if (alter_space_move_indexes(alter, 0, old_space->index_id_max + 1) != 0) return -1; - try { - /* Remember to update schema_version. */ - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + + /* Remember to update schema_version. */ + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); alter_guard.is_active = false; } return 0; +} catch (Exception *) { + return -1; } /** @@ -2552,7 +2529,7 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid) * are modified. */ static int -on_replace_dd_index(struct trigger * /* trigger */, void *event) +on_replace_dd_index(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2653,15 +2630,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) } if (alter_space_move_indexes(alter, 0, iid) != 0) return -1; - try { - if (old_index->def->opts.is_unique) { - (void) new DropConstraintID(alter, - old_def->name); - } - (void) new DropIndex(alter, old_index); - } catch (Exception *e) { - return -1; + + if (old_index->def->opts.is_unique) { + (void) new DropConstraintID(alter, + old_def->name); } + (void) new DropIndex(alter, old_index); } /* Case 2: create an index, if it is simply created. */ if (old_index == NULL && new_tuple != NULL) { @@ -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); }); + 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) { @@ -2707,18 +2678,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) do_new_constraint_id = true; do_drop_constraint_id = true; } - try { - if (do_new_constraint_id) { - (void) new CreateConstraintID( - alter, CONSTRAINT_TYPE_UNIQUE, - index_def->name); - } - if (do_drop_constraint_id) { - (void) new DropConstraintID(alter, - old_def->name); - } - } catch (Exception *e) { - return -1; + if (do_new_constraint_id) { + (void) new CreateConstraintID( + alter, CONSTRAINT_TYPE_UNIQUE, + index_def->name); + } + if (do_drop_constraint_id) { + (void) new DropConstraintID(alter, + old_def->name); } /* * To detect which key parts are optional, @@ -2764,11 +2731,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) return -1; if (index_def_cmp(index_def, old_index->def) == 0) { /* Index is not changed so just move it. */ - try { - (void) new MoveIndex(alter, old_index->def->iid); - } catch (Exception *e) { - return -1; - } + (void) new MoveIndex(alter, old_index->def->iid); } else if (index_def_change_requires_rebuild(old_index, index_def)) { @@ -2782,12 +2745,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) /* * Operation demands an index rebuild. */ - try { - (void) new RebuildIndex(alter, index_def, - old_index->def); - } catch (Exception *e) { - return -1; - } + (void) new RebuildIndex(alter, index_def, + old_index->def); index_def_guard.is_active = false; } else { /* @@ -2795,12 +2754,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) * but we still need to check that tuples stored * in the space conform to the new format. */ - try { - (void) new CheckSpaceFormat(alter); - (void) new ModifyIndex(alter, old_index, index_def); - } catch (Exception *e) { - return -1; - } + (void) new CheckSpaceFormat(alter); + (void) new ModifyIndex(alter, old_index, index_def); index_def_guard.is_active = false; } } @@ -2810,16 +2765,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) */ if (alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1) != 0) return -1; - try { - (void) new MoveCkConstraints(alter); - /* Add an op to update schema_version on commit. */ - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + (void) new MoveCkConstraints(alter); + /* Add an op to update schema_version on commit. */ + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /** @@ -2834,7 +2787,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) * rollback of all transactions following this one. */ static int -on_replace_dd_truncate(struct trigger * /* trigger */, void *event) +on_replace_dd_truncate(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2893,22 +2846,20 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) stmt->row->group_id = GROUP_LOCAL; } - try { - /* - * Recreate all indexes of the truncated space. - */ - for (uint32_t i = 0; i < old_space->index_count; i++) { - struct index *old_index = old_space->index[i]; - (void) new TruncateIndex(alter, old_index->def->iid); - } - - (void) new MoveCkConstraints(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; + /* + * Recreate all indexes of the truncated space. + */ + for (uint32_t i = 0; i < old_space->index_count; i++) { + struct index *old_index = old_space->index[i]; + (void) new TruncateIndex(alter, old_index->def->iid); } + + (void) new MoveCkConstraints(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /* {{{ access control */ @@ -3091,7 +3042,7 @@ user_cache_remove_user(struct trigger *trigger, void * /* event */) } static int -user_cache_alter_user(struct trigger *trigger, void * /* event */) +user_cache_alter_user(struct trigger *trigger, void * /* event */) try { struct tuple *tuple = (struct tuple *)trigger->data; struct user_def *user = user_def_new_from_tuple(tuple); @@ -3099,20 +3050,18 @@ user_cache_alter_user(struct trigger *trigger, void * /* event */) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); /* Can throw if, e.g. too many users. */ - try { - user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + user_cache_replace(user); def_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /** * A trigger invoked on replace in the user table. */ static int -on_replace_dd_user(struct trigger * /* trigger */, void *event) +on_replace_dd_user(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -3132,11 +3081,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) PRIV_C) != 0) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); - try { - (void) user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + (void) user_cache_replace(user); def_guard.is_active = false; struct trigger *on_rollback = txn_alter_trigger_new(user_cache_remove_user, new_tuple); @@ -3188,11 +3133,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) old_user->def->type, PRIV_A) != 0) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); - try { - user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + user_cache_replace(user); def_guard.is_active = false; struct trigger *on_rollback = txn_alter_trigger_new(user_cache_alter_user, old_tuple); @@ -3201,6 +3142,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) txn_stmt_on_rollback(stmt, on_rollback); } return 0; +} catch (Exception *) { + return -1; } /** @@ -4171,7 +4114,7 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event) * with it. */ static int -register_replica(struct trigger *trigger, void * /* event */) +register_replica(struct trigger *trigger, void * /* event */) try { struct tuple *new_tuple = (struct tuple *)trigger->data; uint32_t id; @@ -4184,14 +4127,13 @@ register_replica(struct trigger *trigger, void * /* event */) if (replica != NULL) { replica_set_id(replica, id); } else { - try { - replica = replicaset_add(id, &uuid); - /* Can't throw exceptions from on_commit trigger */ - } catch(Exception *e) { - panic("Can't register replica: %s", e->errmsg); - } + replica = replicaset_add(id, &uuid); } return 0; +} catch(Exception *e) { + /* Can't throw exceptions from on_commit trigger */ + panic("Can't register replica: %s", e->errmsg); + return -1; } static int @@ -5760,7 +5702,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) /** A trigger invoked on replace in the _func_index space. */ static int -on_replace_dd_func_index(struct trigger *trigger, void *event) +on_replace_dd_func_index(struct trigger *trigger, void *event) try { (void) trigger; struct txn *txn = (struct txn *) event; @@ -5839,24 +5781,18 @@ on_replace_dd_func_index(struct trigger *trigger, void *event) auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);}); if (alter_space_move_indexes(alter, 0, index->def->iid) != 0) return -1; - try { - (void) new RebuildFuncIndex(alter, index->def, func); - } catch (Exception *e) { - return -1; - } + (void) new RebuildFuncIndex(alter, index->def, func); if (alter_space_move_indexes(alter, index->def->iid + 1, space->index_id_max + 1) != 0) return -1; - try { - (void) new MoveCkConstraints(alter); - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + (void) new MoveCkConstraints(alter); + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *e) { + return -1; } struct trigger alter_space_on_replace_space = { -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH 0/2] Simplify alter.cc @ 2020-07-08 9:07 Aleksandr Lyapunov 2020-07-08 9:07 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 0 siblings, 1 reply; 6+ messages in thread From: Aleksandr Lyapunov @ 2020-07-08 9:07 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Since we use C++ we need to use in a way that benefits its advantages. As I see, there are tons of try-catch blocks in alter.cc. We should use more modern techniques and approaches. Aleksandr Lyapunov (2): alter: use good c++ style alter: use proper way to marry C and C++ src/box/alter.cc | 418 +++++++++++++++++++++++-------------------------------- 1 file changed, 177 insertions(+), 241 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ 2020-07-08 9:07 [Tarantool-patches] [PATCH 0/2] Simplify alter.cc Aleksandr Lyapunov @ 2020-07-08 9:07 ` Aleksandr Lyapunov 2020-07-08 10:41 ` Timur Safin 2020-07-11 19:53 ` Vladislav Shpilevoy 0 siblings, 2 replies; 6+ messages in thread From: Aleksandr Lyapunov @ 2020-07-08 9:07 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy 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. 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); } /* 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 { struct space *old_space = alter->old_space; bool is_min_field_count_changed; @@ -1988,17 +1984,9 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, new_def = index_def_dup(old_def); index_def_update_optionality(new_def, min_field_count); - try { - (void) new ModifyIndex(alter, old_index, new_def); - } catch (Exception *e) { - return -1; - } + (void) new ModifyIndex(alter, old_index, new_def); } else { - try { - (void) new MoveIndex(alter, old_def->iid); - } catch (Exception *e) { - return -1; - } + (void) new MoveIndex(alter, old_def->iid); } continue; } @@ -2013,20 +2001,14 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin, index_def_update_optionality(new_def, min_field_count); auto guard = make_scoped_guard([=] { index_def_delete(new_def); }); if (!index_def_change_requires_rebuild(old_index, new_def)) - try { - (void) new ModifyIndex(alter, old_index, new_def); - } catch (Exception *e) { - return -1; - } + (void) new ModifyIndex(alter, old_index, new_def); else - try { - (void) new RebuildIndex(alter, new_def, old_def); - } catch (Exception *e) { - return -1; - } + (void) new RebuildIndex(alter, new_def, old_def); guard.is_active = false; } return 0; +} catch (Exception *) { + return -1; } /** @@ -2189,7 +2171,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event) * clumsy, so it is simply not done. */ static int -on_replace_dd_space(struct trigger * /* trigger */, void *event) +on_replace_dd_space(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2469,28 +2451,23 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) old_space->index_count, def->fields, def->field_count); - try { - (void) new CheckSpaceFormat(alter); - (void) new ModifySpace(alter, def); - (void) new RebuildCkConstraints(alter); - } catch (Exception *e) { - return -1; - } + (void) new CheckSpaceFormat(alter); + (void) new ModifySpace(alter, def); + (void) new RebuildCkConstraints(alter); def_guard.is_active = false; /* Create MoveIndex ops for all space indexes. */ if (alter_space_move_indexes(alter, 0, old_space->index_id_max + 1) != 0) return -1; - try { - /* Remember to update schema_version. */ - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + + /* Remember to update schema_version. */ + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); alter_guard.is_active = false; } return 0; +} catch (Exception *) { + return -1; } /** @@ -2552,7 +2529,7 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid) * are modified. */ static int -on_replace_dd_index(struct trigger * /* trigger */, void *event) +on_replace_dd_index(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2653,15 +2630,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) } if (alter_space_move_indexes(alter, 0, iid) != 0) return -1; - try { - if (old_index->def->opts.is_unique) { - (void) new DropConstraintID(alter, - old_def->name); - } - (void) new DropIndex(alter, old_index); - } catch (Exception *e) { - return -1; + + if (old_index->def->opts.is_unique) { + (void) new DropConstraintID(alter, + old_def->name); } + (void) new DropIndex(alter, old_index); } /* Case 2: create an index, if it is simply created. */ if (old_index == NULL && new_tuple != NULL) { @@ -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); }); + 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) { @@ -2707,18 +2678,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) do_new_constraint_id = true; do_drop_constraint_id = true; } - try { - if (do_new_constraint_id) { - (void) new CreateConstraintID( - alter, CONSTRAINT_TYPE_UNIQUE, - index_def->name); - } - if (do_drop_constraint_id) { - (void) new DropConstraintID(alter, - old_def->name); - } - } catch (Exception *e) { - return -1; + if (do_new_constraint_id) { + (void) new CreateConstraintID( + alter, CONSTRAINT_TYPE_UNIQUE, + index_def->name); + } + if (do_drop_constraint_id) { + (void) new DropConstraintID(alter, + old_def->name); } /* * To detect which key parts are optional, @@ -2764,11 +2731,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) return -1; if (index_def_cmp(index_def, old_index->def) == 0) { /* Index is not changed so just move it. */ - try { - (void) new MoveIndex(alter, old_index->def->iid); - } catch (Exception *e) { - return -1; - } + (void) new MoveIndex(alter, old_index->def->iid); } else if (index_def_change_requires_rebuild(old_index, index_def)) { @@ -2782,12 +2745,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) /* * Operation demands an index rebuild. */ - try { - (void) new RebuildIndex(alter, index_def, - old_index->def); - } catch (Exception *e) { - return -1; - } + (void) new RebuildIndex(alter, index_def, + old_index->def); index_def_guard.is_active = false; } else { /* @@ -2795,12 +2754,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) * but we still need to check that tuples stored * in the space conform to the new format. */ - try { - (void) new CheckSpaceFormat(alter); - (void) new ModifyIndex(alter, old_index, index_def); - } catch (Exception *e) { - return -1; - } + (void) new CheckSpaceFormat(alter); + (void) new ModifyIndex(alter, old_index, index_def); index_def_guard.is_active = false; } } @@ -2810,16 +2765,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) */ if (alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1) != 0) return -1; - try { - (void) new MoveCkConstraints(alter); - /* Add an op to update schema_version on commit. */ - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + (void) new MoveCkConstraints(alter); + /* Add an op to update schema_version on commit. */ + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /** @@ -2834,7 +2787,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) * rollback of all transactions following this one. */ static int -on_replace_dd_truncate(struct trigger * /* trigger */, void *event) +on_replace_dd_truncate(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -2893,22 +2846,20 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) stmt->row->group_id = GROUP_LOCAL; } - try { - /* - * Recreate all indexes of the truncated space. - */ - for (uint32_t i = 0; i < old_space->index_count; i++) { - struct index *old_index = old_space->index[i]; - (void) new TruncateIndex(alter, old_index->def->iid); - } - - (void) new MoveCkConstraints(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; + /* + * Recreate all indexes of the truncated space. + */ + for (uint32_t i = 0; i < old_space->index_count; i++) { + struct index *old_index = old_space->index[i]; + (void) new TruncateIndex(alter, old_index->def->iid); } + + (void) new MoveCkConstraints(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /* {{{ access control */ @@ -3091,7 +3042,7 @@ user_cache_remove_user(struct trigger *trigger, void * /* event */) } static int -user_cache_alter_user(struct trigger *trigger, void * /* event */) +user_cache_alter_user(struct trigger *trigger, void * /* event */) try { struct tuple *tuple = (struct tuple *)trigger->data; struct user_def *user = user_def_new_from_tuple(tuple); @@ -3099,20 +3050,18 @@ user_cache_alter_user(struct trigger *trigger, void * /* event */) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); /* Can throw if, e.g. too many users. */ - try { - user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + user_cache_replace(user); def_guard.is_active = false; return 0; +} catch (Exception *) { + return -1; } /** * A trigger invoked on replace in the user table. */ static int -on_replace_dd_user(struct trigger * /* trigger */, void *event) +on_replace_dd_user(struct trigger * /* trigger */, void *event) try { struct txn *txn = (struct txn *) event; struct txn_stmt *stmt = txn_current_stmt(txn); @@ -3132,11 +3081,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) PRIV_C) != 0) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); - try { - (void) user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + (void) user_cache_replace(user); def_guard.is_active = false; struct trigger *on_rollback = txn_alter_trigger_new(user_cache_remove_user, new_tuple); @@ -3188,11 +3133,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) old_user->def->type, PRIV_A) != 0) return -1; auto def_guard = make_scoped_guard([=] { free(user); }); - try { - user_cache_replace(user); - } catch (Exception *e) { - return -1; - } + user_cache_replace(user); def_guard.is_active = false; struct trigger *on_rollback = txn_alter_trigger_new(user_cache_alter_user, old_tuple); @@ -3201,6 +3142,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) txn_stmt_on_rollback(stmt, on_rollback); } return 0; +} catch (Exception *) { + return -1; } /** @@ -4171,7 +4114,7 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event) * with it. */ static int -register_replica(struct trigger *trigger, void * /* event */) +register_replica(struct trigger *trigger, void * /* event */) try { struct tuple *new_tuple = (struct tuple *)trigger->data; uint32_t id; @@ -4184,14 +4127,13 @@ register_replica(struct trigger *trigger, void * /* event */) if (replica != NULL) { replica_set_id(replica, id); } else { - try { - replica = replicaset_add(id, &uuid); - /* Can't throw exceptions from on_commit trigger */ - } catch(Exception *e) { - panic("Can't register replica: %s", e->errmsg); - } + replica = replicaset_add(id, &uuid); } return 0; +} catch(Exception *e) { + /* Can't throw exceptions from on_commit trigger */ + panic("Can't register replica: %s", e->errmsg); + return -1; } static int @@ -5760,7 +5702,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) /** A trigger invoked on replace in the _func_index space. */ static int -on_replace_dd_func_index(struct trigger *trigger, void *event) +on_replace_dd_func_index(struct trigger *trigger, void *event) try { (void) trigger; struct txn *txn = (struct txn *) event; @@ -5839,24 +5781,18 @@ on_replace_dd_func_index(struct trigger *trigger, void *event) auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);}); if (alter_space_move_indexes(alter, 0, index->def->iid) != 0) return -1; - try { - (void) new RebuildFuncIndex(alter, index->def, func); - } catch (Exception *e) { - return -1; - } + (void) new RebuildFuncIndex(alter, index->def, func); if (alter_space_move_indexes(alter, index->def->iid + 1, space->index_id_max + 1) != 0) return -1; - try { - (void) new MoveCkConstraints(alter); - (void) new UpdateSchemaVersion(alter); - alter_space_do(stmt, alter); - } catch (Exception *e) { - return -1; - } + (void) new MoveCkConstraints(alter); + (void) new UpdateSchemaVersion(alter); + alter_space_do(stmt, alter); scoped_guard.is_active = false; return 0; +} catch (Exception *e) { + return -1; } struct trigger alter_space_on_replace_space = { -- 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ 2020-07-08 9:07 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov @ 2020-07-08 10:41 ` Timur Safin 2020-07-11 19:53 ` Vladislav Shpilevoy 1 sibling, 0 replies; 6+ messages in thread From: Timur Safin @ 2020-07-08 10:41 UTC (permalink / raw) To: 'Aleksandr Lyapunov', tarantool-patches; +Cc: v.shpilevoy : 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ 2020-07-08 9:07 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 2020-07-08 10:41 ` Timur Safin @ 2020-07-11 19:53 ` Vladislav Shpilevoy 1 sibling, 0 replies; 6+ messages in thread From: Vladislav Shpilevoy @ 2020-07-11 19:53 UTC (permalink / raw) To: Aleksandr Lyapunov, tarantool-patches 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-11 19:53 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-08 8:43 [Tarantool-patches] [PATCH 0/2] Simplify alter.cc Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 1/2] alter: use good c++ style Aleksandr Lyapunov 2020-07-08 8:43 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 2020-07-08 9:07 [Tarantool-patches] [PATCH 0/2] Simplify alter.cc Aleksandr Lyapunov 2020-07-08 9:07 ` [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++ Aleksandr Lyapunov 2020-07-08 10:41 ` Timur Safin 2020-07-11 19:53 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox