[tarantool-patches] Re: [PATCH v2 3/3] Multibyte characters support

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Please, consider 9 comments.

1. Seems like you again sent the patch using diff copy-paste into mail 
client. Please, don't do
that. Such "method"
- destroys all tabs, converting them to spaces, which count != tab width,
- skips commit message body.

Now I look at the letter in commits mail list.

2. Please, write a commit message body and ref GitHub issue using 
'Linked with #NNNN' or
'Part of #NNNN'. But actually your patch closes it, as I think.

3. In lbox_tuple_field_by_path you calculate path len when a field is 
not found - please, try
use already calculated len from this place:
>   	size_t path_len;
> -	const char *path = lua_tolstring(L, 2, &path_len);
> +	path = lua_tolstring(L, 2, &path_len);
4. I can not build the branch:
> [ 15%] Building C object test/unit/CMakeFiles/heap.test.dir/heap.c.o
> In file included from 
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/lib/json/path.c:32:
> /Users/v.shpilevoy/Work/Repositories/tarantool/src/lib/json/path.h:37:10: 
> fatal error: 'malloc.h' file not found
> #include <malloc.h>
>          ^~~~~~~~~~
> [ 15%] Built target api

>   		if (index < 0) {
>   not_found:
> +		if (!path)
> +			goto exit_not_found;
> +		uint32_t path_len = strlen(path);
5. The indentation looks broken.

6. Please, try to simplify the function. It looks very complex with 3 
labels and a strange "mark".

> /** + * Checks is multibyte character whose first byte + * is pointed 
> to by mb_str is alphabetic. + * NOTE: You have to clean global context 
> + * with mbtowc(NULL, 0, 0); before first call
7. Where you found this info? I can not find it.

> +/** + * Counts user-string sign count in mb_str_size bytes + * @param 
> mb_str + * @param mb_str_size + * @return sign count + */ +static 
> inline int +mbtowc_count(struct json_path_parser *parser, const char 
> *mb_str, size_t mb_str_len) +{ + char src[mb_str_len+1];

8. I very-very do not like allocation of arrays with variable length on 
a stack. Please, do not do that.
It is bad even if you use this function for errors only. And please try 
to remove this function at all. I
propose to calculate symbols count in struct json_path_parser. For 
example, you can add a member
symbol_count, and increase it during each json_path_next call.

9. Looks, like you did not add a tests on complex cases, which we have 
discussed verbally, when a space
format field has name looking like JSON paths. Please, add them. And I 
recommend you do not hurry. Speed
of the patch pushing into master does not depend on speed of patch 
resending. And remember, that it will be
very hot code, and it must extremely optimized.