[tarantool-patches] Re: [PATCH 2/4] tuple: rework update error reporting

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [19/09/16 09:58]:
> +static inline const char *
> +update_op_field_str(const struct update_op *op)
> +{
> +	if (op->field_no >= 0)
> +		return tt_sprintf("%d", op->field_no + TUPLE_INDEX_BASE);
> +	else
> +		return tt_sprintf("%d", op->field_no);
> +}
> +
> +static inline int
> +update_err_arg_type(const struct update_op *op, const char *needed_type)
> +{
> +	diag_set(ClientError, ER_UPDATE_ARG_TYPE, op->opcode,
> +		 update_op_field_str(op), needed_type);
> +	return -1;
> +}
> +
> +static inline int
> +update_err_int_overflow(const struct update_op *op)
> +{
> +	diag_set(ClientError, ER_UPDATE_INTEGER_OVERFLOW, op->opcode,
> +		 update_op_field_str(op));
> +	return -1;
> +}
> +
> +static inline int
> +update_err_decimal_overflow(const struct update_op *op)
> +{
> +	diag_set(ClientError, ER_UPDATE_DECIMAL_OVERFLOW, op->opcode,
> +		 update_op_field_str(op));
> +	return -1;
> +}
> +
> +static inline int
> +update_err_splice_bound(const struct update_op *op)
> +{
> +	diag_set(ClientError, ER_UPDATE_SPLICE, update_op_field_str(op),
> +		 "offset is out of bound");
> +	return -1;
> +}
> +
> +static inline int
> +update_err_no_such_field(const struct update_op *op)
> +{
> +	diag_set(ClientError, ER_NO_SUCH_FIELD_NO, op->field_no >= 0 ?
> +		 TUPLE_INDEX_BASE + op->field_no : op->field_no);
> +	return -1;
> +}
> +
> +static inline int
> +update_err(const struct update_op *op, const char *reason)
> +{
> +	diag_set(ClientError, ER_UPDATE_FIELD, update_op_field_str(op),
> +		 reason);
> +	return -1;
> +}
> +
> +static inline int
> +update_err_double(const struct update_op *op)
> +{
> +	return update_err(op, "double update of the same field");
> +}

If these functions are static, you don't have to prefix them with
upate_, better give them more descriptive names, like,
set_double_update_op_error(), set_no_such_field_error(), etc, 

Re dropping index_base: this is an incompatible change. Peter
Gulutzan is very much in favour of indexing fields from 1 in all
frontends.  I was initially thinking about JavaScript when added index_base. 
Plus the C/C++ plugin API is zero-based.
By removing it form error messages you make the code marginally
simpler (since you keep it in the API), and more confusing IMO.

I'd either remove index_base support altogether (an incompatible
change, old programs would break), or keep it in the messages.

>  		switch(opcode) {
>  		case '+':
> -			int96_add(&arg1.int96, &arg2.int96);
> +			int96_add(&arg.int96, &value.int96);

Arg and value are both general, ambiguous names. Both variables are
update operation arguments. Both variables are values (as opposed
to literals or something else). 
If you want to make the names more clear and specific,
highlighting which side is left and which is right, you could
call them "field", and "param", or "receiver", and "param", 
lhs and rhs (left hand side and right hand side, like in C++
operators).

>  		}
>  		if (lowest_type == AT_DOUBLE) {
> -			/* result is DOUBLE */
>  			ret->type = AT_DOUBLE;
>  			ret->dbl = c;
>  		} else {
> -			/* result is FLOAT */
>  			assert(lowest_type == AT_FLOAT);
>  			ret->type = AT_FLOAT;
>  			ret->flt = (float)c;

I find it barbarian practice - to delete other people comments,
unless they have became obsolete. Whoever wrote this comment
had a reason for it.

>  do_op_delete(struct tuple_update *update, struct update_op *op)
>  {
> -	if (op_adjust_field_no(update, op, rope_size(update->rope)))
> +	if (op_adjust_field_no(op, rope_size(update->rope)) != 0)
>  		return -1;
>  	uint32_t delete_count = op->arg.del.count;
>  
>  	if ((uint64_t) op->field_no + delete_count > rope_size(update->rope))
>  		delete_count = rope_size(update->rope) - op->field_no;
>  
> -	if (delete_count == 0) {
> -		diag_set(ClientError, ER_UPDATE_FIELD,
> -			 update->index_base + op->field_no,
> -			 "cannot delete 0 fields");
> -		return -1;
> -	}

Why did you delete this piece? If op_adjust_field_no checks for
it, please replace with an assert.

-- 
Konstantin Osipov, Moscow, Russia