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

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

[Aleksandr Lyapunov]

Since we use C++ we need to use in a way that benefits its advantages.
As I see, there are tons of try-catch blocks in alter.cc. We should use more modern techniques and approaches.

Aleksandr Lyapunov (2):
  alter: use good c++ style
  alter: use proper way to marry C and C++

 src/box/alter.cc | 418 +++++++++++++++++++++++--------------------------------
 1 file changed, 177 insertions(+), 241 deletions(-)

-- 
2.7.4

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

[Aleksandr Lyapunov]

We should use noecept where it's appropriate.
1.It informs a developer he could not care about exception safety
2.It instructs a compiler to check throws if possible

We should override for compile-time check of misprints in names
of methods and types of arguments.

Part of #5153
---
 src/box/alter.cc | 184 +++++++++++++++++++++++++++----------------------------
 1 file changed, 92 insertions(+), 92 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bb42548..1a7949e 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -760,7 +760,7 @@ struct alter_space;
 
 class AlterSpaceOp {
 public:
-	AlterSpaceOp(struct alter_space *alter);
+	explicit AlterSpaceOp(struct alter_space *alter);
 
 	/** Link in alter_space::ops. */
 	struct rlist link;
@@ -769,7 +769,7 @@ public:
 	 * the space definition and/or key list that will be used
 	 * for creating the new space. Must not yield or fail.
 	 */
-	virtual void alter_def(struct alter_space * /* alter */) {}
+	virtual void alter_def(struct alter_space * /* alter */) noexcept {}
 	/**
 	 * Called after creating a new space. Used for performing
 	 * long-lasting operations, such as index rebuild or format
@@ -783,21 +783,21 @@ public:
 	 * state to the new space (e.g. move unchanged indexes).
 	 * Must not yield or fail.
 	 */
-	virtual void alter(struct alter_space * /* alter */) {}
+	virtual void alter(struct alter_space * /* alter */) noexcept {}
 	/**
 	 * Called after the change has been successfully written
 	 * to WAL. Must not fail.
 	 */
 	virtual void commit(struct alter_space * /* alter */,
-			    int64_t /* signature */) {}
+			    int64_t /* signature */) noexcept {}
 	/**
 	 * Called in case a WAL error occurred. It is supposed to undo
 	 * the effect of AlterSpaceOp::prepare and AlterSpaceOp::alter.
 	 * Must not fail.
 	 */
-	virtual void rollback(struct alter_space * /* alter */) {}
+	virtual void rollback(struct alter_space * /* alter */) noexcept {}
 
-	virtual ~AlterSpaceOp() {}
+	virtual ~AlterSpaceOp() noexcept {}
 
 	void *operator new(size_t size)
 	{
@@ -979,7 +979,7 @@ struct mh_i32_t *AlterSpaceLock::registry;
  * Replace the old space with a new one in the space cache.
  */
 static int
-alter_space_commit(struct trigger *trigger, void *event)
+alter_space_commit(struct trigger *trigger, void *event) noexcept
 {
 	struct txn *txn = (struct txn *) event;
 	struct alter_space *alter = (struct alter_space *) trigger->data;
@@ -997,12 +997,8 @@ alter_space_commit(struct trigger *trigger, void *event)
 	 * indexes into their new places.
 	 */
 	class AlterSpaceOp *op;
-	try {
-		rlist_foreach_entry(op, &alter->ops, link)
-			op->commit(alter, signature);
-	} catch (Exception *e) {
-		return -1;
-	}
+	rlist_foreach_entry(op, &alter->ops, link)
+		op->commit(alter, signature);
 
 	alter->new_space = NULL; /* for alter_space_delete(). */
 	/*
@@ -1024,7 +1020,7 @@ alter_space_commit(struct trigger *trigger, void *event)
  * alter_space_commit() failure (unlikely)
  */
 static int
-alter_space_rollback(struct trigger *trigger, void * /* event */)
+alter_space_rollback(struct trigger *trigger, void * /* event */) noexcept
 {
 	struct alter_space *alter = (struct alter_space *) trigger->data;
 	/* Rollback alter ops */
@@ -1183,9 +1179,9 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space *alter)
 class CheckSpaceFormat: public AlterSpaceOp
 {
 public:
-	CheckSpaceFormat(struct alter_space *alter)
+	explicit CheckSpaceFormat(struct alter_space *alter)
 		:AlterSpaceOp(alter) {}
-	virtual void prepare(struct alter_space *alter);
+	void prepare(struct alter_space *alter) override;
 };
 
 void
@@ -1217,15 +1213,15 @@ public:
 	 * names into an old dictionary and deletes new one.
 	 */
 	struct tuple_dictionary *new_dict;
-	virtual void alter_def(struct alter_space *alter);
-	virtual void alter(struct alter_space *alter);
-	virtual void rollback(struct alter_space *alter);
-	virtual ~ModifySpace();
+	void alter_def(struct alter_space *alter) noexcept override;
+	void alter(struct alter_space *alter) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
+	~ModifySpace() noexcept override;
 };
 
 /** Amend the definition of the new space. */
 void
-ModifySpace::alter_def(struct alter_space *alter)
+ModifySpace::alter_def(struct alter_space *alter) noexcept
 {
 	/*
 	 * Use the old dictionary for the new space, because
@@ -1244,7 +1240,7 @@ ModifySpace::alter_def(struct alter_space *alter)
 }
 
 void
-ModifySpace::alter(struct alter_space *alter)
+ModifySpace::alter(struct alter_space *alter) noexcept
 {
 	/*
 	 * Move new names into an old dictionary, which already is
@@ -1255,12 +1251,12 @@ ModifySpace::alter(struct alter_space *alter)
 }
 
 void
-ModifySpace::rollback(struct alter_space *alter)
+ModifySpace::rollback(struct alter_space *alter) noexcept
 {
 	tuple_dictionary_swap(alter->new_space->def->dict, new_dict);
 }
 
-ModifySpace::~ModifySpace()
+ModifySpace::~ModifySpace() noexcept
 {
 	if (new_dict != NULL)
 		tuple_dictionary_unref(new_dict);
@@ -1276,9 +1272,9 @@ public:
 	DropIndex(struct alter_space *alter, struct index *index)
 		:AlterSpaceOp(alter), old_index(index) {}
 	struct index *old_index;
-	virtual void alter_def(struct alter_space *alter);
-	virtual void prepare(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t lsn);
+	void alter_def(struct alter_space *alter) noexcept override;
+	void prepare(struct alter_space *alter) override;
+	void commit(struct alter_space *alter, int64_t lsn) noexcept override;
 };
 
 /*
@@ -1286,7 +1282,7 @@ public:
  * the new index from it.
  */
 void
-DropIndex::alter_def(struct alter_space * /* alter */)
+DropIndex::alter_def(struct alter_space * /* alter */) noexcept
 {
 	rlist_del_entry(old_index->def, link);
 }
@@ -1300,7 +1296,7 @@ DropIndex::prepare(struct alter_space *alter)
 }
 
 void
-DropIndex::commit(struct alter_space *alter, int64_t signature)
+DropIndex::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	(void)alter;
 	index_commit_drop(old_index, signature);
@@ -1318,18 +1314,18 @@ public:
 		:AlterSpaceOp(alter), iid(iid_arg) {}
 	/** id of the index on the move. */
 	uint32_t iid;
-	virtual void alter(struct alter_space *alter);
-	virtual void rollback(struct alter_space *alter);
+	void alter(struct alter_space *alter) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
 };
 
 void
-MoveIndex::alter(struct alter_space *alter)
+MoveIndex::alter(struct alter_space *alter) noexcept
 {
 	space_swap_index(alter->old_space, alter->new_space, iid, iid);
 }
 
 void
-MoveIndex::rollback(struct alter_space *alter)
+MoveIndex::rollback(struct alter_space *alter) noexcept
 {
 	space_swap_index(alter->old_space, alter->new_space, iid, iid);
 }
@@ -1360,23 +1356,23 @@ public:
 	struct index *old_index;
 	struct index *new_index;
 	struct index_def *new_index_def;
-	virtual void alter_def(struct alter_space *alter);
-	virtual void alter(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t lsn);
-	virtual void rollback(struct alter_space *alter);
-	virtual ~ModifyIndex();
+	void alter_def(struct alter_space *alter) noexcept override;
+	void alter(struct alter_space *alter) noexcept override;
+	void commit(struct alter_space *alter, int64_t lsn) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
+	~ModifyIndex() noexcept override;
 };
 
 /** Update the definition of the new space */
 void
-ModifyIndex::alter_def(struct alter_space *alter)
+ModifyIndex::alter_def(struct alter_space *alter) noexcept
 {
 	rlist_del_entry(old_index->def, link);
 	index_def_list_add(&alter->key_list, new_index_def);
 }
 
 void
-ModifyIndex::alter(struct alter_space *alter)
+ModifyIndex::alter(struct alter_space *alter) noexcept
 {
 	new_index = space_index(alter->new_space, new_index_def->iid);
 	assert(old_index->def->iid == new_index->def->iid);
@@ -1392,14 +1388,14 @@ ModifyIndex::alter(struct alter_space *alter)
 }
 
 void
-ModifyIndex::commit(struct alter_space *alter, int64_t signature)
+ModifyIndex::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	(void)alter;
 	index_commit_modify(new_index, signature);
 }
 
 void
-ModifyIndex::rollback(struct alter_space *alter)
+ModifyIndex::rollback(struct alter_space *alter) noexcept
 {
 	/*
 	 * Restore indexes.
@@ -1411,7 +1407,7 @@ ModifyIndex::rollback(struct alter_space *alter)
 	index_update_def(old_index);
 }
 
-ModifyIndex::~ModifyIndex()
+ModifyIndex::~ModifyIndex() noexcept
 {
 	index_def_delete(new_index_def);
 }
@@ -1427,15 +1423,15 @@ public:
 	CreateIndex(struct alter_space *alter, struct index_def *def)
 		:AlterSpaceOp(alter), new_index(NULL), new_index_def(def)
 	{}
-	virtual void alter_def(struct alter_space *alter);
-	virtual void prepare(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t lsn);
-	virtual ~CreateIndex();
+	void alter_def(struct alter_space *alter) noexcept override;
+	void prepare(struct alter_space *alter) override;
+	void commit(struct alter_space *alter, int64_t lsn) noexcept override;
+	~CreateIndex() noexcept override;
 };
 
 /** Add definition of the new key to the new space def. */
 void
-CreateIndex::alter_def(struct alter_space *alter)
+CreateIndex::alter_def(struct alter_space *alter) noexcept
 {
 	index_def_list_add(&alter->key_list, new_index_def);
 }
@@ -1475,7 +1471,7 @@ CreateIndex::prepare(struct alter_space *alter)
 }
 
 void
-CreateIndex::commit(struct alter_space *alter, int64_t signature)
+CreateIndex::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	(void) alter;
 	assert(new_index != NULL);
@@ -1483,7 +1479,7 @@ CreateIndex::commit(struct alter_space *alter, int64_t signature)
 	new_index = NULL;
 }
 
-CreateIndex::~CreateIndex()
+CreateIndex::~CreateIndex() noexcept
 {
 	if (new_index != NULL)
 		index_abort_create(new_index);
@@ -1516,15 +1512,15 @@ public:
 	struct index_def *new_index_def;
 	/** Old index index_def. */
 	struct index_def *old_index_def;
-	virtual void alter_def(struct alter_space *alter);
-	virtual void prepare(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t signature);
-	virtual ~RebuildIndex();
+	void alter_def(struct alter_space *alter) noexcept override;
+	void prepare(struct alter_space *alter) override;
+	void commit(struct alter_space *alter, int64_t signature) noexcept override;
+	~RebuildIndex() noexcept override;
 };
 
 /** Add definition of the new key to the new space def. */
 void
-RebuildIndex::alter_def(struct alter_space *alter)
+RebuildIndex::alter_def(struct alter_space *alter) noexcept
 {
 	rlist_del_entry(old_index_def, link);
 	index_def_list_add(&alter->key_list, new_index_def);
@@ -1540,7 +1536,7 @@ RebuildIndex::prepare(struct alter_space *alter)
 }
 
 void
-RebuildIndex::commit(struct alter_space *alter, int64_t signature)
+RebuildIndex::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	struct index *old_index = space_index(alter->old_space,
 					      old_index_def->iid);
@@ -1551,7 +1547,7 @@ RebuildIndex::commit(struct alter_space *alter, int64_t signature)
 	new_index = NULL;
 }
 
-RebuildIndex::~RebuildIndex()
+RebuildIndex::~RebuildIndex() noexcept
 {
 	if (new_index != NULL)
 		index_abort_create(new_index);
@@ -1595,9 +1591,10 @@ public:
 	TruncateIndex(struct alter_space *alter, uint32_t iid)
 		: AlterSpaceOp(alter), iid(iid),
 		  old_index(NULL), new_index(NULL) {}
-	virtual void prepare(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t signature);
-	virtual ~TruncateIndex();
+	void prepare(struct alter_space *alter) override;
+	void commit(struct alter_space *alter,
+	            int64_t signature) noexcept override;
+	~TruncateIndex() override;
 };
 
 void
@@ -1627,7 +1624,7 @@ TruncateIndex::prepare(struct alter_space *alter)
 }
 
 void
-TruncateIndex::commit(struct alter_space *alter, int64_t signature)
+TruncateIndex::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	(void)alter;
 	index_commit_drop(old_index, signature);
@@ -1635,7 +1632,7 @@ TruncateIndex::commit(struct alter_space *alter, int64_t signature)
 	new_index = NULL;
 }
 
-TruncateIndex::~TruncateIndex()
+TruncateIndex::~TruncateIndex() noexcept
 {
 	if (new_index == NULL)
 		return;
@@ -1652,14 +1649,14 @@ class UpdateSchemaVersion: public AlterSpaceOp
 public:
 	UpdateSchemaVersion(struct alter_space * alter)
 		:AlterSpaceOp(alter) {}
-	virtual void alter(struct alter_space *alter);
+	void alter(struct alter_space *alter) noexcept override;
 };
 
 void
-UpdateSchemaVersion::alter(struct alter_space *alter)
+UpdateSchemaVersion::alter(struct alter_space *alter) noexcept
 {
-    (void)alter;
-    ++schema_version;
+	(void) alter;
+	++schema_version;
 }
 
 /**
@@ -1679,10 +1676,10 @@ public:
 	RebuildCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter),
 		ck_constraint(RLIST_HEAD_INITIALIZER(ck_constraint)) {}
 	struct rlist ck_constraint;
-	virtual void prepare(struct alter_space *alter);
-	virtual void alter(struct alter_space *alter);
-	virtual void rollback(struct alter_space *alter);
-	virtual ~RebuildCkConstraints();
+	void prepare(struct alter_space *alter) override;
+	void alter(struct alter_space *alter) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
+	~RebuildCkConstraints() override;
 };
 
 void
@@ -1711,18 +1708,18 @@ RebuildCkConstraints::space_swap_ck_constraint(struct space *old_space,
 }
 
 void
-RebuildCkConstraints::alter(struct alter_space *alter)
+RebuildCkConstraints::alter(struct alter_space *alter) noexcept
 {
 	space_swap_ck_constraint(alter->old_space, alter->new_space);
 }
 
 void
-RebuildCkConstraints::rollback(struct alter_space *alter)
+RebuildCkConstraints::rollback(struct alter_space *alter) noexcept
 {
 	space_swap_ck_constraint(alter->new_space, alter->old_space);
 }
 
-RebuildCkConstraints::~RebuildCkConstraints()
+RebuildCkConstraints::~RebuildCkConstraints() noexcept
 {
 	struct ck_constraint *old_ck_constraint, *tmp;
 	rlist_foreach_entry_safe(old_ck_constraint, &ck_constraint, link, tmp) {
@@ -1749,8 +1746,8 @@ class MoveCkConstraints: public AlterSpaceOp
 				      struct space *new_space);
 public:
 	MoveCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {}
-	virtual void alter(struct alter_space *alter);
-	virtual void rollback(struct alter_space *alter);
+	void alter(struct alter_space *alter) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
 };
 
 void
@@ -1764,13 +1761,13 @@ MoveCkConstraints::space_swap_ck_constraint(struct space *old_space,
 }
 
 void
-MoveCkConstraints::alter(struct alter_space *alter)
+MoveCkConstraints::alter(struct alter_space *alter) noexcept
 {
 	space_swap_ck_constraint(alter->old_space, alter->new_space);
 }
 
 void
-MoveCkConstraints::rollback(struct alter_space *alter)
+MoveCkConstraints::rollback(struct alter_space *alter) noexcept
 {
 	space_swap_ck_constraint(alter->new_space, alter->old_space);
 }
@@ -1829,11 +1826,12 @@ public:
 		if (new_id == NULL)
 			diag_raise();
 	}
-	virtual void prepare(struct alter_space *alter);
-	virtual void alter(struct alter_space *alter);
-	virtual void rollback(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter, int64_t signature);
-	virtual ~CreateConstraintID();
+	void prepare(struct alter_space *alter) override;
+	void alter(struct alter_space *alter) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
+	void commit(struct alter_space *alter,
+	            int64_t signature) noexcept override;
+	~CreateConstraintID() noexcept override;
 };
 
 void
@@ -1845,7 +1843,7 @@ CreateConstraintID::prepare(struct alter_space *alter)
 }
 
 void
-CreateConstraintID::alter(struct alter_space *alter)
+CreateConstraintID::alter(struct alter_space *alter) noexcept
 {
 	/* Alter() can't fail, so can't just throw an error. */
 	if (space_add_constraint_id(alter->old_space, new_id) != 0)
@@ -1853,14 +1851,15 @@ CreateConstraintID::alter(struct alter_space *alter)
 }
 
 void
-CreateConstraintID::rollback(struct alter_space *alter)
+CreateConstraintID::rollback(struct alter_space *alter) noexcept
 {
 	space_delete_constraint_id(alter->new_space, new_id->name);
 	new_id = NULL;
 }
 
 void
-CreateConstraintID::commit(struct alter_space *alter, int64_t signature)
+CreateConstraintID::commit(struct alter_space *alter,
+                           int64_t signature) noexcept
 {
 	(void) alter;
 	(void) signature;
@@ -1871,7 +1870,7 @@ CreateConstraintID::commit(struct alter_space *alter, int64_t signature)
 	new_id = NULL;
 }
 
-CreateConstraintID::~CreateConstraintID()
+CreateConstraintID::~CreateConstraintID() noexcept
 {
 	if (new_id != NULL)
 		constraint_id_delete(new_id);
@@ -1886,19 +1885,20 @@ public:
 	DropConstraintID(struct alter_space *alter, const char *name)
 		:AlterSpaceOp(alter), old_id(NULL), name(name)
 	{}
-	virtual void alter(struct alter_space *alter);
-	virtual void commit(struct alter_space *alter , int64_t signature);
-	virtual void rollback(struct alter_space *alter);
+	void alter(struct alter_space *alter) noexcept override;
+	void commit(struct alter_space *alter,
+	            int64_t signature) noexcept override;
+	void rollback(struct alter_space *alter) noexcept override;
 };
 
 void
-DropConstraintID::alter(struct alter_space *alter)
+DropConstraintID::alter(struct alter_space *alter) noexcept
 {
 	old_id = space_pop_constraint_id(alter->old_space, name);
 }
 
 void
-DropConstraintID::commit(struct alter_space *alter, int64_t signature)
+DropConstraintID::commit(struct alter_space *alter, int64_t signature) noexcept
 {
 	(void) alter;
 	(void) signature;
@@ -1906,7 +1906,7 @@ DropConstraintID::commit(struct alter_space *alter, int64_t signature)
 }
 
 void
-DropConstraintID::rollback(struct alter_space *alter)
+DropConstraintID::rollback(struct alter_space *alter) noexcept
 {
 	if (space_add_constraint_id(alter->new_space, old_id) != 0) {
 		panic("Can't recover after constraint drop rollback (out of "
-- 
2.7.4

[Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C and C++

[Aleksandr Lyapunov]

We should not try-catch something that must not throw.
It's dumb.

We should use function-try-block to convert C++ function to C.
It's short, simple and does not create big intends.

Closes #5153
---
 src/box/alter.cc | 234 ++++++++++++++++++++-----------------------------------
 1 file changed, 85 insertions(+), 149 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 1a7949e..9a0192f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1025,12 +1025,8 @@ alter_space_rollback(struct trigger *trigger, void * /* event */) noexcept
 	struct alter_space *alter = (struct alter_space *) trigger->data;
 	/* Rollback alter ops */
 	class AlterSpaceOp *op;
-	try {
-		rlist_foreach_entry(op, &alter->ops, link) {
-			op->rollback(alter);
-		}
-	} catch (Exception *e) {
-		return -1;
+	rlist_foreach_entry(op, &alter->ops, link) {
+		op->rollback(alter);
 	}
 	/* Rebuild index maps once for all indexes. */
 	space_fill_index_map(alter->old_space);
@@ -1965,7 +1961,7 @@ on_create_space_rollback(struct trigger *trigger, void *event)
  */
 int
 alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
-			 uint32_t end)
+			 uint32_t end) try
 {
 	struct space *old_space = alter->old_space;
 	bool is_min_field_count_changed;
@@ -1988,17 +1984,9 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 				new_def = index_def_dup(old_def);
 				index_def_update_optionality(new_def,
 							     min_field_count);
-				try {
-					(void) new ModifyIndex(alter, old_index, new_def);
-				} catch (Exception *e) {
-					return -1;
-				}
+				(void) new ModifyIndex(alter, old_index, new_def);
 			} else {
-				try {
-					(void) new MoveIndex(alter, old_def->iid);
-				} catch (Exception *e) {
-					return -1;
-				}
+				(void) new MoveIndex(alter, old_def->iid);
 			}
 			continue;
 		}
@@ -2013,20 +2001,14 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 		index_def_update_optionality(new_def, min_field_count);
 		auto guard = make_scoped_guard([=] { index_def_delete(new_def); });
 		if (!index_def_change_requires_rebuild(old_index, new_def))
-			try {
-				(void) new ModifyIndex(alter, old_index, new_def);
-			} catch (Exception *e) {
-				return -1;
-			}
+			(void) new ModifyIndex(alter, old_index, new_def);
 		else
-			try {
-				(void) new RebuildIndex(alter, new_def, old_def);
-			} catch (Exception *e) {
-				return -1;
-			}
+			(void) new RebuildIndex(alter, new_def, old_def);
 		guard.is_active = false;
 	}
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /**
@@ -2189,7 +2171,7 @@ on_drop_view_rollback(struct trigger *trigger, void *event)
  * clumsy, so it is simply not done.
  */
 static int
-on_replace_dd_space(struct trigger * /* trigger */, void *event)
+on_replace_dd_space(struct trigger * /* trigger */, void *event) try
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
@@ -2469,28 +2451,23 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 						     old_space->index_count,
 						     def->fields,
 						     def->field_count);
-		try {
-			(void) new CheckSpaceFormat(alter);
-			(void) new ModifySpace(alter, def);
-			(void) new RebuildCkConstraints(alter);
-		} catch (Exception *e) {
-			return -1;
-		}
+		(void) new CheckSpaceFormat(alter);
+		(void) new ModifySpace(alter, def);
+		(void) new RebuildCkConstraints(alter);
 		def_guard.is_active = false;
 		/* Create MoveIndex ops for all space indexes. */
 		if (alter_space_move_indexes(alter, 0,
 		    old_space->index_id_max + 1) != 0)
 			return -1;
-		try {
-			/* Remember to update schema_version. */
-			(void) new UpdateSchemaVersion(alter);
-			alter_space_do(stmt, alter);
-		} catch (Exception *e) {
-			return -1;
-		}
+
+		/* Remember to update schema_version. */
+		(void) new UpdateSchemaVersion(alter);
+		alter_space_do(stmt, alter);
 		alter_guard.is_active = false;
 	}
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /**
@@ -2552,7 +2529,7 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
  *   are modified.
  */
 static int
-on_replace_dd_index(struct trigger * /* trigger */, void *event)
+on_replace_dd_index(struct trigger * /* trigger */, void *event) try
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
@@ -2653,15 +2630,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 		}
 		if (alter_space_move_indexes(alter, 0, iid) != 0)
 			return -1;
-		try {
-			if (old_index->def->opts.is_unique) {
-				(void) new DropConstraintID(alter,
-							    old_def->name);
-			}
-			(void) new DropIndex(alter, old_index);
-		} catch (Exception *e) {
-			return -1;
+
+		if (old_index->def->opts.is_unique) {
+			(void) new DropConstraintID(alter,
+						    old_def->name);
 		}
+		(void) new DropIndex(alter, old_index);
 	}
 	/* Case 2: create an index, if it is simply created. */
 	if (old_index == NULL && new_tuple != NULL) {
@@ -2672,17 +2646,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 		if (def == NULL)
 			return -1;
 		index_def_update_optionality(def, alter->new_min_field_count);
-		try {
-			if (def->opts.is_unique) {
-				(void) new CreateConstraintID(
-					alter, iid == 0 ? CONSTRAINT_TYPE_PK :
-					CONSTRAINT_TYPE_UNIQUE, def->name);
-			}
-			(void) new CreateIndex(alter, def);
-		} catch (Exception *e) {
-			index_def_delete(def);
-			return -1;
+		auto guard = make_scoped_guard([=] { index_def_delete(def); });
+		if (def->opts.is_unique) {
+			(void) new CreateConstraintID(
+				alter, iid == 0 ? CONSTRAINT_TYPE_PK :
+				CONSTRAINT_TYPE_UNIQUE, def->name);
 		}
+		(void) new CreateIndex(alter, def);
+		guard.is_active = false;
 	}
 	/* Case 3 and 4: check if we need to rebuild index data. */
 	if (old_index != NULL && new_tuple != NULL) {
@@ -2707,18 +2678,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			do_new_constraint_id = true;
 			do_drop_constraint_id = true;
 		}
-		try {
-			if (do_new_constraint_id) {
-				(void) new CreateConstraintID(
-					alter, CONSTRAINT_TYPE_UNIQUE,
-					index_def->name);
-			}
-			if (do_drop_constraint_id) {
-				(void) new DropConstraintID(alter,
-							    old_def->name);
-			}
-		} catch (Exception *e) {
-			return -1;
+		if (do_new_constraint_id) {
+			(void) new CreateConstraintID(
+				alter, CONSTRAINT_TYPE_UNIQUE,
+				index_def->name);
+		}
+		if (do_drop_constraint_id) {
+			(void) new DropConstraintID(alter,
+						    old_def->name);
 		}
 		/*
 		 * To detect which key parts are optional,
@@ -2764,11 +2731,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			return -1;
 		if (index_def_cmp(index_def, old_index->def) == 0) {
 			/* Index is not changed so just move it. */
-			try {
-				(void) new MoveIndex(alter, old_index->def->iid);
-			} catch (Exception *e) {
-				return -1;
-			}
+			(void) new MoveIndex(alter, old_index->def->iid);
 
 		} else if (index_def_change_requires_rebuild(old_index,
 							     index_def)) {
@@ -2782,12 +2745,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			/*
 			 * Operation demands an index rebuild.
 			 */
-			try {
-				(void) new RebuildIndex(alter, index_def,
-							old_index->def);
-			} catch (Exception *e) {
-				return -1;
-			}
+			(void) new RebuildIndex(alter, index_def,
+						old_index->def);
 			index_def_guard.is_active = false;
 		} else {
 			/*
@@ -2795,12 +2754,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			 * but we still need to check that tuples stored
 			 * in the space conform to the new format.
 			 */
-			try {
-				(void) new CheckSpaceFormat(alter);
-				(void) new ModifyIndex(alter, old_index, index_def);
-			} catch (Exception *e) {
-				return -1;
-			}
+			(void) new CheckSpaceFormat(alter);
+			(void) new ModifyIndex(alter, old_index, index_def);
 			index_def_guard.is_active = false;
 		}
 	}
@@ -2810,16 +2765,14 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	 */
 	if (alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1) != 0)
 		return -1;
-	try {
-		(void) new MoveCkConstraints(alter);
-		/* Add an op to update schema_version on commit. */
-		(void) new UpdateSchemaVersion(alter);
-		alter_space_do(stmt, alter);
-	} catch (Exception *e) {
-		return -1;
-	}
+	(void) new MoveCkConstraints(alter);
+	/* Add an op to update schema_version on commit. */
+	(void) new UpdateSchemaVersion(alter);
+	alter_space_do(stmt, alter);
 	scoped_guard.is_active = false;
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /**
@@ -2834,7 +2787,7 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
  * rollback of all transactions following this one.
  */
 static int
-on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
+on_replace_dd_truncate(struct trigger * /* trigger */, void *event) try
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
@@ -2893,22 +2846,20 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
 		stmt->row->group_id = GROUP_LOCAL;
 	}
 
-	try {
-		/*
-		 * Recreate all indexes of the truncated space.
-		 */
-		for (uint32_t i = 0; i < old_space->index_count; i++) {
-			struct index *old_index = old_space->index[i];
-			(void) new TruncateIndex(alter, old_index->def->iid);
-		}
-
-		(void) new MoveCkConstraints(alter);
-		alter_space_do(stmt, alter);
-	} catch (Exception *e) {
-		return -1;
+	/*
+	 * Recreate all indexes of the truncated space.
+	 */
+	for (uint32_t i = 0; i < old_space->index_count; i++) {
+		struct index *old_index = old_space->index[i];
+		(void) new TruncateIndex(alter, old_index->def->iid);
 	}
+
+	(void) new MoveCkConstraints(alter);
+	alter_space_do(stmt, alter);
 	scoped_guard.is_active = false;
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /* {{{ access control */
@@ -3091,7 +3042,7 @@ user_cache_remove_user(struct trigger *trigger, void * /* event */)
 }
 
 static int
-user_cache_alter_user(struct trigger *trigger, void * /* event */)
+user_cache_alter_user(struct trigger *trigger, void * /* event */) try
 {
 	struct tuple *tuple = (struct tuple *)trigger->data;
 	struct user_def *user = user_def_new_from_tuple(tuple);
@@ -3099,20 +3050,18 @@ user_cache_alter_user(struct trigger *trigger, void * /* event */)
 		return -1;
 	auto def_guard = make_scoped_guard([=] { free(user); });
 	/* Can throw if, e.g. too many users. */
-	try {
-		user_cache_replace(user);
-	} catch (Exception *e) {
-		return -1;
-	}
+	user_cache_replace(user);
 	def_guard.is_active = false;
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /**
  * A trigger invoked on replace in the user table.
  */
 static int
-on_replace_dd_user(struct trigger * /* trigger */, void *event)
+on_replace_dd_user(struct trigger * /* trigger */, void *event) try
 {
 	struct txn *txn = (struct txn *) event;
 	struct txn_stmt *stmt = txn_current_stmt(txn);
@@ -3132,11 +3081,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 				 PRIV_C) != 0)
 			return -1;
 		auto def_guard = make_scoped_guard([=] { free(user); });
-		try {
-			(void) user_cache_replace(user);
-		} catch (Exception *e) {
-			return -1;
-		}
+		(void) user_cache_replace(user);
 		def_guard.is_active = false;
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(user_cache_remove_user, new_tuple);
@@ -3188,11 +3133,7 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 				 old_user->def->type, PRIV_A) != 0)
 			return -1;
 		auto def_guard = make_scoped_guard([=] { free(user); });
-		try {
-			user_cache_replace(user);
-		} catch (Exception *e) {
-			return -1;
-		}
+		user_cache_replace(user);
 		def_guard.is_active = false;
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(user_cache_alter_user, old_tuple);
@@ -3201,6 +3142,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event)
 		txn_stmt_on_rollback(stmt, on_rollback);
 	}
 	return 0;
+} catch (Exception *) {
+	return -1;
 }
 
 /**
@@ -4171,7 +4114,7 @@ on_replace_dd_schema(struct trigger * /* trigger */, void *event)
  * with it.
  */
 static int
-register_replica(struct trigger *trigger, void * /* event */)
+register_replica(struct trigger *trigger, void * /* event */) try
 {
 	struct tuple *new_tuple = (struct tuple *)trigger->data;
 	uint32_t id;
@@ -4184,14 +4127,13 @@ register_replica(struct trigger *trigger, void * /* event */)
 	if (replica != NULL) {
 		replica_set_id(replica, id);
 	} else {
-		try {
-			replica = replicaset_add(id, &uuid);
-			/* Can't throw exceptions from on_commit trigger */
-		} catch(Exception *e) {
-			panic("Can't register replica: %s", e->errmsg);
-		}
+		replica = replicaset_add(id, &uuid);
 	}
 	return 0;
+} catch(Exception *e) {
+	/* Can't throw exceptions from on_commit trigger */
+	panic("Can't register replica: %s", e->errmsg);
+	return -1;
 }
 
 static int
@@ -5760,7 +5702,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 
 /** A trigger invoked on replace in the _func_index space. */
 static int
-on_replace_dd_func_index(struct trigger *trigger, void *event)
+on_replace_dd_func_index(struct trigger *trigger, void *event) try
 {
 	(void) trigger;
 	struct txn *txn = (struct txn *) event;
@@ -5839,24 +5781,18 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
 	auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);});
 	if (alter_space_move_indexes(alter, 0, index->def->iid) != 0)
 		return -1;
-	try {
-		(void) new RebuildFuncIndex(alter, index->def, func);
-	} catch (Exception *e) {
-		return -1;
-	}
+	(void) new RebuildFuncIndex(alter, index->def, func);
 	if (alter_space_move_indexes(alter, index->def->iid + 1,
 				     space->index_id_max + 1) != 0)
 		return -1;
-	try {
-		(void) new MoveCkConstraints(alter);
-		(void) new UpdateSchemaVersion(alter);
-		alter_space_do(stmt, alter);
-	} catch (Exception *e) {
-		return -1;
-	}
+	(void) new MoveCkConstraints(alter);
+	(void) new UpdateSchemaVersion(alter);
+	alter_space_do(stmt, alter);
 
 	scoped_guard.is_active = false;
 	return 0;
+} catch (Exception *e) {
+	return -1;
 }
 
 struct trigger alter_space_on_replace_space = {
-- 
2.7.4

Re: [Tarantool-patches] [PATCH 0/2] Simplify alter.cc

[Timur Safin]

I'm very much infavor to use less verbose try-function blocks - 
they significantly reduce verbosity of a C++ code which should 
be in between C++ and C functions. So indeed - that's good idiom.

Although I suspect `noexcept` in C++17 is not that much zero-cost 
abstraction (it should be verified with all supported compilers) but looks 
like it's not adding any extra overhead which we would not have at the moment.

But I have some (debatable) notes about lambda usage. See in patches...

Timur

: From: Aleksandr Lyapunov
: Subject: [Tarantool-patches] [PATCH 0/2] Simplify alter.cc
: 
: Since we use C++ we need to use in a way that benefits its advantages.
: As I see, there are tons of try-catch blocks in alter.cc. We should use
: more modern techniques and approaches.
: 
: Aleksandr Lyapunov (2):
:   alter: use good c++ style
:   alter: use proper way to marry C and C++
: 
:  src/box/alter.cc | 418 +++++++++++++++++++++++---------------------------
: -----
:  1 file changed, 177 insertions(+), 241 deletions(-)
: 
: --
: 2.7.4

Re: [Tarantool-patches] [PATCH 0/2] Simplify alter.cc

[Aleksandr Lyapunov]

+Ilya Kosarev

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

On 08.07.2020 12:07, Aleksandr Lyapunov wrote:
> Since we use C++ we need to use in a way that benefits its advantages.
> As I see, there are tons of try-catch blocks in alter.cc. We should use more modern techniques and approaches.
>
> Aleksandr Lyapunov (2):
>    alter: use good c++ style
>    alter: use proper way to marry C and C++
>
>   src/box/alter.cc | 418 +++++++++++++++++++++++--------------------------------
>   1 file changed, 177 insertions(+), 241 deletions(-)
>

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

[Aleksandr Lyapunov]

Since we use C++ we need to use in a way that benefits its advantages.
As I see, there are tons of try-catch blocks in alter.cc. We should use more modern techniques and approaches.

Aleksandr Lyapunov (2):
  alter: use good c++ style
  alter: use proper way to marry C and C++

 src/box/alter.cc | 418 +++++++++++++++++++++++--------------------------------
 1 file changed, 177 insertions(+), 241 deletions(-)

-- 
2.7.4