[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

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

[Vladislav Shpilevoy]

Thanks for the patch!

See 3 comments below.

On 08/07/2020 11:07, Aleksandr Lyapunov wrote:
> We should use noecept where it's appropriate.

1. noecept -> noexcept.

2. I am getting build errors:

/Users/gerold/Work/Repositories/tarantool/src/box/alter.cc:1635:16: error: function previously declared with an implicit exception specification redeclared with an explicit exception specification
      [-Werror,-Wimplicit-exception-spec-mismatch]
TruncateIndex::~TruncateIndex() noexcept
               ^
/Users/gerold/Work/Repositories/tarantool/src/box/alter.cc:1597:2: note: previous declaration is here
        ~TruncateIndex() override;
        ^
/Users/gerold/Work/Repositories/tarantool/src/box/alter.cc:1722:23: error: function previously declared with an implicit exception specification redeclared with an explicit exception specification
      [-Werror,-Wimplicit-exception-spec-mismatch]
RebuildCkConstraints::~RebuildCkConstraints() noexcept
                      ^
/Users/gerold/Work/Repositories/tarantool/src/box/alter.cc:1682:2: note: previous declaration is here
        ~RebuildCkConstraints() override;
        ^
2 errors generated.

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

3. This change seems not to be related.

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

[Aleksandr Lyapunov]

Hi! Thanks for the review! all good except one more question below?

On 11.07.2020 22:53, Vladislav Shpilevoy wrote:
>
>>   void
>> -UpdateSchemaVersion::alter(struct alter_space *alter)
>> +UpdateSchemaVersion::alter(struct alter_space *alter) noexcept
>>   {
>> -    (void)alter;
>> -    ++schema_version;
>> +	(void) alter;
>> +	++schema_version;
> 3. This change seems not to be related.
What are you suggesting to do with it? create separate commit? or don't 
touch it? I hope I haven't to have an gh issue for that.

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

[Vladislav Shpilevoy]

On 13.07.2020 15:36, Aleksandr Lyapunov wrote:
> Hi! Thanks for the review! all good except one more question below?
> 
> On 11.07.2020 22:53, Vladislav Shpilevoy wrote:
>>
>>>   void
>>> -UpdateSchemaVersion::alter(struct alter_space *alter)
>>> +UpdateSchemaVersion::alter(struct alter_space *alter) noexcept
>>>   {
>>> -    (void)alter;
>>> -    ++schema_version;
>>> +    (void) alter;
>>> +    ++schema_version;
>> 3. This change seems not to be related.
> What are you suggesting to do with it? create separate commit? or don't touch it? I hope I haven't to have an gh issue for that.

I would just drop it. It is not needed. Not visually nor
functionally nothing changes. But whatever. Keep if you want.

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

[Timur Safin]

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


: @@ -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;

This is C++, not C - we could simply omit argument name, and there is 
no need to dereference argument to make compiler shut

: @@ -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;

The same comment is applicable here - we could simply omit argument name


: @@ -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;

And here... 

:  	index_commit_drop(old_index, signature);
: @@ -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;

And here ...

: -    ++schema_version;
: +	(void) alter;
: +	++schema_version;

But here we could ask _very important question_ (:)) do we use spaces
or tabs (like in C) for indenting C++ sources? Looks like Tabs are not yet
used in this file, thus no need to enforce the different style.


:  }
: 
:  /**
: @@ -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;

Here we could omit names of both arguments ...

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

The same comment is applicable here also...

Best Regards,
Timur

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

[Vladislav Shpilevoy]

> : @@ -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;
> 
> The same comment is applicable here - we could simply omit argument name

Argument name omission reduces code readability. We have already had
that discussion in Lua because of luacheck requiring to drop some
arguments. But it does not make the code better really.

> : -    ++schema_version;
> : +	(void) alter;
> : +	++schema_version;
> 
> But here we could ask _very important question_ (:)) do we use spaces
> or tabs (like in C) for indenting C++ sources? Looks like Tabs are not yet
> used in this file, thus no need to enforce the different style.

Alter.cc uses tabs only. Except this place probably.

[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 2/2] alter: use proper way to marry C and C++

[Timur Safin]

: From: Aleksandr Lyapunov
: Subject: [Tarantool-patches] [PATCH 2/2] alter: use proper way to marry C
: and C++
: 
: 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.
: 
...
: @@ -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);
: });

I knew a lot of C++ code guidelines which specifically forbid 
capturing all arguments in C++ lambdas via copy or reference
and insist on explicit names passage. I do believe it makes some 
sense. What about to apply the same policy here and after? 


: +		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) {

Also, this operator new usage as wrapper around rlist operations 
looks ... not very much idiomatic, and unexpected at least (we do 
see new allocating memory which is instantly forgotten. WTF the 
immediate reaction). If we introduce here some sort of reference
counted allocations what about to wrap it somehow differently, 
say via templatized make_rlist_object<T> or something like that?
(even simple macros would looks clearer, IMVHO).

I'm not insisting on that approach, just saying...

Timur

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

[Vladislav Shpilevoy]

Thanks for the patch!

On 08/07/2020 11:07, Aleksandr Lyapunov wrote:
> We should not try-catch something that must not throw.
> It's dumb.

Yeah, well. It is not so. It was done for a certain reason - expecting
future conversion to C, to reduce diff. Because when converted to C,
this code will check individual places, not entire function like SQLite
does or C++ with exceptions.

Moreover, even if we will keep C++, we will never use exceptions more
than now for sure. So the error checking will stay in C way as I expect.

I don't know about other things Timur told about how to capture things
in lambda, and how is it forbidden, but I do know that the exceptions are
often forbidden, and for reasons.

So I don't really think it is a good idea to extend the code throwing
exceptions.

Except a few places which actually didn't throw anything ever, these ones
you are right to fix. For example, on_commit and on_rollback triggers -
they never throw indeed. They can panic, at most.

Much better solution would be to eliminate exceptions from there. Not
mask them even more. You wouldn't need try-catch and noexcept at all.

See 2 comments below.

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

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

>  	/* 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

2. This one I don't like. It makes harder to eliminate exceptions in future. Because
will need to check each function call in this function to see if it throws. Before
your patch we could locate such places by simple grep by 'try' and 'catch'.

However it is arguable when it comes to constructors. Anyway we will change them and
won't miss these places. But when it comes to function calls, it is not good. For
example, when you move alter_space_do() out of explicit try-catch.

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

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

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