From: "Ilya Kosarev" <i.kosarev@tarantool.org> To: "Aleksandr Lyapunov" <alyapunov@tarantool.org> Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style Date: Tue, 04 Aug 2020 13:04:04 +0300 [thread overview] Message-ID: <1596535444.410127167@f712.i.mail.ru> (raw) In-Reply-To: <1596534955.779570951@f731.i.mail.ru> [-- Attachment #1: Type: text/plain, Size: 20054 bytes --] Also there are 4 places where tabs were replaced by spaces, marked them below. >Вторник, 4 августа 2020, 12:55 +03:00 от Ilya Kosarev <i.kosarev@tarantool.org>: > >Hi! > >Yes, this patch seems to be nice. > >See 1 comment below. >>Понедельник, 13 июля 2020, 17:18 +03:00 от Aleksandr Lyapunov < alyapunov@tarantool.org >: >> >>We should use noexcept 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 | 180 +++++++++++++++++++++++++++---------------------------- >> 1 file changed, 90 insertions(+), 90 deletions(-) >> >>diff --git a/src/box/alter.cc b/src/box/alter.cc >>index 249cee2..15a51d0 100644 >>--- a/src/box/alter.cc >>+++ b/src/box/alter.cc >>@@ -765,7 +765,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; >>@@ -774,7 +774,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 >>@@ -788,21 +788,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) >> { >>@@ -984,7 +984,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; >>@@ -1002,12 +1002,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); >I guess this change should be in >«alter: do not catch exceptions when it's not necessary» commit. >> >> alter->new_space = NULL; /* for alter_space_delete(). */ >> /* >>@@ -1029,7 +1025,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 */ >>@@ -1188,9 +1184,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 >>@@ -1222,15 +1218,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 >>@@ -1249,7 +1245,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 >>@@ -1260,12 +1256,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); >>@@ -1281,9 +1277,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; >> }; >> >> /* >>@@ -1291,7 +1287,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); >> } >>@@ -1305,7 +1301,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); >>@@ -1323,18 +1319,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); >> } >>@@ -1365,23 +1361,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); >>@@ -1397,14 +1393,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. >>@@ -1416,7 +1412,7 @@ ModifyIndex::rollback(struct alter_space *alter) >> index_update_def(old_index); >> } >> >>-ModifyIndex::~ModifyIndex() >>+ModifyIndex::~ModifyIndex() noexcept >> { >> index_def_delete(new_index_def); >> } >>@@ -1432,15 +1428,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); >> } >>@@ -1480,7 +1476,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); >>@@ -1488,7 +1484,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); >>@@ -1521,15 +1517,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); >>@@ -1545,7 +1541,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); >>@@ -1556,7 +1552,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); >>@@ -1600,9 +1596,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; * Here on a new line. >>+ ~TruncateIndex() noexcept override; >> }; >> >> void >>@@ -1632,7 +1629,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); >>@@ -1640,7 +1637,7 @@ TruncateIndex::commit(struct alter_space *alter, int64_t signature) >> new_index = NULL; >> } >> >>-TruncateIndex::~TruncateIndex() >>+TruncateIndex::~TruncateIndex() noexcept >> { >> if (new_index == NULL) >> return; >>@@ -1657,11 +1654,11 @@ 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; >>@@ -1684,10 +1681,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() noexcept override; >> }; >> >> void >>@@ -1716,18 +1713,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) { >>@@ -1754,8 +1751,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 >>@@ -1769,13 +1766,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); >> } >>@@ -1834,11 +1831,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; * Here on a new line. >>+ ~CreateConstraintID() noexcept override; >> }; >> >> void >>@@ -1850,7 +1848,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) >>@@ -1858,14 +1856,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 * Here on a new line. >> { >> (void) alter; >> (void) signature; >>@@ -1876,7 +1875,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); >>@@ -1891,19 +1890,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; * Here on a new line. >>+ 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; >>@@ -1911,7 +1911,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 >> > > >-- >Ilya Kosarev > -- Ilya Kosarev [-- Attachment #2: Type: text/html, Size: 25400 bytes --]
next prev parent reply other threads:[~2020-08-04 10:04 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-13 14:18 [Tarantool-patches] [PATCH v2 0/3] Simplify alter.cc Aleksandr Lyapunov 2020-07-13 14:18 ` [Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style Aleksandr Lyapunov 2020-08-04 9:55 ` Ilya Kosarev 2020-08-04 10:04 ` Ilya Kosarev [this message] 2020-07-13 14:18 ` [Tarantool-patches] [PATCH v2 2/3] alter: fix codestyle Aleksandr Lyapunov 2020-08-04 10:10 ` Ilya Kosarev 2020-07-13 14:18 ` [Tarantool-patches] [PATCH v2 3/3] alter: do not catch exceptions when it's not necessary Aleksandr Lyapunov 2020-07-13 18:34 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1596535444.410127167@f712.i.mail.ru \ --to=i.kosarev@tarantool.org \ --cc=alyapunov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox