[Tarantool-patches] [PATCH v5 18/52] sql: introduce mem_concat()

[Mergen Imeev]

Thank you for the review! My answer and diff below.

On Thu, Apr 15, 2021 at 01:04:27AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the discussion!
> 
> >>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> >>> index b417c1007..2d76ef88d 100644
> >>> --- a/src/box/sql/mem.c
> >>> +++ b/src/box/sql/mem.c
> >>> @@ -326,6 +326,70 @@ mem_move(struct Mem *to, struct Mem *from)
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static bool
> >>> +is_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
> >>> +	       enum field_type type)
> >>
> >> 1. Functions called 'is_*' never should change anything.
> >>
> > Fixed. Renamed to check_result_null().
> > 
> >> Another question is why do you even need it? It is used in a single place,
> >> where it could be just inlined. And is not used in a place, where it could
> >> be applied.
> >>
> > I added it here since it was the first commit, which used it. This functions
> > will be used in all arithmetic and bitwise operations with two operands.
> 
> I hope it is not too late to rename it to try_return_null()? Because
> sorry, but 'check' also has a meaning - it is just like 'is' (does not
> change anything), but sets a diag. At least this is what I see in the
> code usually, and try to force in the new patches.

Fixed, thank you! Also fixed in next patches.

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index e06f964bd..2f2f859e3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -182,8 +182,8 @@ mem_move(struct Mem *to, struct Mem *from)
 }
 
 static bool
-check_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
-		  enum field_type type)
+try_return_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
+		enum field_type type)
 {
 	mem_clear(result);
 	result->field_type = type;
@@ -195,7 +195,7 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
 {
 	assert(result != b);
 	if (a != result) {
-		if (check_result_null(a, b, result, FIELD_TYPE_STRING))
+		if (try_return_null(a, b, result, FIELD_TYPE_STRING))
 			return 0;
 	} else {
 		if (((a->flags | b->flags) & MEM_Null) != 0) {