[Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style
Ilya Kosarev
i.kosarev at tarantool.org
Tue Aug 4 13:04:04 MSK 2020
Also there are 4 places where tabs were replaced by spaces,
marked them below.
>Вторник, 4 августа 2020, 12:55 +03:00 от Ilya Kosarev <i.kosarev at tarantool.org>:
>
>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;
*
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200804/821974fe/attachment.html>
More information about the Tarantool-patches
mailing list