Re: [Tarantool-patches] [PATCH v1 1/2] sql: disallow explicit cast of BOOLEAN to number

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> index 6b95e41d3..5c44bfdfc 100644
> --- a/src/box/sql/mem.c
> +++ b/src/box/sql/mem.c
> @@ -683,24 +674,23 @@ str_to_bool(struct Mem *mem)
>  {
>  	assert(mem->type == MEM_TYPE_STR);
>  	char *str = mem->z;
> +	uint32_t len = mem->n;
>  	bool b;
>  	const char *str_true = "TRUE";
>  	const char *str_false = "FALSE";
>  	uint32_t len_true = strlen(str_true);
>  	uint32_t len_false = strlen(str_false);
>  
> -	for (; str[0] == ' '; str++);
> -	if (strncasecmp(str, str_true, len_true) == 0) {
> +	for (; isspace(str[0]); str++, len--);
> +	for (; isspace(str[len - 1]); len--);
> +	if (len != len_true && len != len_false)
> +		return -1;
> +
> +	if (len == len_true && strncasecmp(str, str_true, len) == 0)
>  		b = true;
> -		str += len_true;
> -	} else if (strncasecmp(str, str_false, len_false) == 0) {
> +	else if (len == len_false && strncasecmp(str, str_false, len) == 0)
>  		b = false;
> -		str += len_false;
> -	} else {
> -		return -1;
> -	}
> -	for (; str[0] == ' '; str++);
> -	if (str[0] != '\0')
> +	else
>  		return -1;

1. Why did you change str_to_bool() if the patch is only about
numbers <-> bool?

>  	mem_set_bool(mem, b);
>  	return 0;
> @@ -1074,19 +1036,11 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
>  	case FIELD_TYPE_INTEGER:
>  		return mem_to_int(mem);
>  	case FIELD_TYPE_BOOLEAN:
> -		switch (mem->type) {
> -		case MEM_TYPE_BOOL:
> +		if (mem->type == MEM_TYPE_BOOL)
>  			return 0;
> -		case MEM_TYPE_INT:
> -		case MEM_TYPE_UINT:
> -			return int_to_bool(mem);
> -		case MEM_TYPE_STR:
> +		if (mem->type == MEM_TYPE_STR)
>  			return str_to_bool(mem);
> -		case MEM_TYPE_DOUBLE:
> -			return double_to_bool(mem);
> -		default:
> -			return -1;

2. I would propose to keep the switch-case. Otherwise you are
going to jump back and forth between if and switch when these
places will be changed again.