[Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style
Ilya Kosarev
i.kosarev at tarantool.org
Tue Aug 4 12:55:55 MSK 2020
Hi!
Yes, this patch seems to be nice.
See 1 comment below.
>Понедельник, 13 июля 2020, 17:18 +03:00 от Aleksandr Lyapunov <alyapunov at 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;
>+ ~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;
>+ ~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
> {
> (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;
>+ 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200804/702cf997/attachment.html>
More information about the Tarantool-patches
mailing list