[Tarantool-patches] [PATCH v2 0/3] Simplify alter.cc

[Tarantool-patches] [PATCH v2 0/3] Simplify alter.cc

[Aleksandr Lyapunov]

Since we use C++ we need to use in a way that benefits its advantages.
At least we should remove useless try-catch blocks.

GH issue: https://github.com/tarantool/tarantool/issues/5153
GH branch: https://github.com/tarantool/tarantool/tree/alyapunov/gh-5153-simplify-alter-cc

v2: after-CR fixes, remove function-try block.

Aleksandr Lyapunov (3):
  alter: use good c++ style
  alter: fix codestyle
  alter: do not catch exceptions when it's not necessary

 src/box/alter.cc | 192 +++++++++++++++++++++++++++----------------------------
 1 file changed, 94 insertions(+), 98 deletions(-)

-- 
2.7.4

[Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style

[Aleksandr Lyapunov]

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);
 
 	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

[Tarantool-patches] [PATCH v2 2/3] alter: fix codestyle

[Aleksandr Lyapunov]

---
 src/box/alter.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 15a51d0..9b2b8e8 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1660,8 +1660,8 @@ public:
 void
 UpdateSchemaVersion::alter(struct alter_space *alter) noexcept
 {
-    (void)alter;
-    ++schema_version;
+	(void) alter;
+	++schema_version;
 }
 
 /**
-- 
2.7.4

[Tarantool-patches] [PATCH v2 3/3] alter: do not catch exceptions when it's not necessary

[Aleksandr Lyapunov]

Some method are guarantee to be noexcept, we should not try them.

Closes #5153
---
 src/box/alter.cc | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 9b2b8e8..1807c33 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1030,12 +1030,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);
-- 
2.7.4

Re: [Tarantool-patches] [PATCH v2 3/3] alter: do not catch exceptions when it's not necessary

[Vladislav Shpilevoy]

On 13.07.2020 16:18, Aleksandr Lyapunov wrote:
> Some method are guarantee to be noexcept, we should not try them.
> 
> Closes #5153
> ---
>  src/box/alter.cc | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 9b2b8e8..1807c33 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1030,12 +1030,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);

My comment to the previous version still remains:

	1. This one is good. Try-catch wasn't needed here. But it looks like related to
	the previous commit. Not to this one.

You did exactly the same change for on_commit triggers in
the first commit. Why did you move on_rollback change into a
separate commit?

Re: [Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 19193 bytes --]


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;
>+ ~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
 

[-- Attachment #2: Type: text/html, Size: 22375 bytes --]

Re: [Tarantool-patches] [PATCH v2 1/3] alter: use good c++ style

[Ilya Kosarev]

[-- 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 --]

Re: [Tarantool-patches] [PATCH v2 2/3] alter: fix codestyle

[Ilya Kosarev]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]


As far as i see, there are more places in alter.cc with wrong
codestyle (spaces instead of tabs) but i guess those are the worst.
  
>Понедельник, 13 июля 2020, 17:18 +03:00 от Aleksandr Lyapunov <alyapunov@tarantool.org>:
> 
>---
> src/box/alter.cc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/src/box/alter.cc b/src/box/alter.cc
>index 15a51d0..9b2b8e8 100644
>--- a/src/box/alter.cc
>+++ b/src/box/alter.cc
>@@ -1660,8 +1660,8 @@ public:
> void
> UpdateSchemaVersion::alter(struct alter_space *alter) noexcept
> {
>- (void)alter;
>- ++schema_version;
>+ (void) alter;
>+ ++schema_version;
> }
> 
> /**
>--
>2.7.4
>  
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 1215 bytes --]