<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">See below 5 comments. And see at the travis: <a href="https://travis-ci.org/tarantool/tarantool/builds/359964649?utm_source=github_status&utm_medium=notification" class="">https://travis-ci.org/tarantool/tarantool/builds/359964649?utm_source=github_status&utm_medium=notification</a> - seems like the patch does not work. box/alter.test.lua with unicode and your own tests are failed.<div class=""><div><br class=""><blockquote type="cite" class=""><div class="">29 марта 2018 г., в 21:04, Kirill Shcherbatov <<a href="mailto:kshcherbatov@tarantool.org" class="">kshcherbatov@tarantool.org</a>> написал(а):</div><br class="Apple-interchange-newline"><div class=""><div class="">From eb16ee6ea2d2ca5801b6b09c403c6e2f23984bd9 Mon Sep 17 00:00:00 2001<br class="">From: Kirill Shcherbatov <<a href="mailto:kshcherbatov@tarantool.org" class="">kshcherbatov@tarantool.org</a>><br class="">Date: Thu, 29 Mar 2018 21:03:21 +0300<br class="">Subject: [PATCH] Multibyte characters support<br class=""><br class="">---<br class=""> src/box/lua/tuple.c | 1 -<br class=""> src/lib/json/path.c | 35 ++++++++++++++++++++++++++++++-----<br class=""> src/lib/json/path.h | 2 ++<br class=""> test/engine/tuple.result | 16 ++++++++++++++--<br class=""> test/engine/tuple.test.lua | 5 ++++-<br class=""> 5 files changed, 50 insertions(+), 9 deletions(-)<br class=""><br class="">diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c<br class="">index 99b9ff2..c3a435b 100644<br class="">--- a/src/box/lua/tuple.c<br class="">+++ b/src/box/lua/tuple.c<br class="">@@ -44,6 +46,26 @@ strntoull(const char *src, int len) {<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return value;<br class=""> }<br class=""><br class=""></div></div></blockquote><div><br class=""></div><div>1. Please, add comments what the function does, what arguments gets, what returns.</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+static inline size_t<br class="">+mbsize(const char *str, size_t str_size)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>mbstate_t ps;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>memset(&ps, 0, sizeof(ps));<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>mbrlen(NULL,0,&ps);<br class=""></div></div></blockquote><div><br class=""></div><div>2. I do not like this always-zero ps. Why you can not use mblen instead of mbrlen?</div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>return mbrlen(str, str_size, &ps);<br class="">+}<br class="">+<br class="">+static inline int<br class="">+mb2wcisalpha(char *mb_char, size_t mb_char_size)<br class="">+{<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>assert(mb_char_size < 5);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>wchar_t buff[2];<br class=""></div></div></blockquote><div><br class=""></div><div>3. Sorry, I still can not understand, why do you need 2 wchar, if here only</div><div>one is checked? Why do you use mbsrtowcs instead of <span style="font-family: DejaVuSansMono, "DejaVu Sans Mono", courier, monospace; font-size: 12.800000190734863px; white-space: nowrap; background-color: rgb(255, 255, 255);" class="">mbrtowc.</span></div><br class=""><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>mbstate_t ps;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>memset(&ps, 0, sizeof(ps));</div></div></blockquote><blockquote type="cite" class=""><div class=""><div class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>mbsrtowcs(buff, (const char **)&mb_char, mb_char_size + 1, &ps);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>return iswalpha((wint_t)buff[0]);<br class="">+}<br class="">+<br class=""> /**<br class=""> * Parse string identifier in quotes. Parser either stops right<br class=""> * after the closing quote, or returns an error position.<br class="">@@ -126,17 +148,20 @@ json_parse_identifier(struct json_path_parser *parser,<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>const char *str = pos;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>char c = *pos;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>/* First symbol can not be digit. */<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>if (!isalpha(c) && c != '_')<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>return pos - parser->src + 1;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>size_t mb_size = 0;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>if ((mb_size = mbsize(pos, end - pos)) && !mb2wcisalpha((char *)pos, mb_size) && c != '_')<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>return pos - parser->src + mb_size;<br class=""></div></div></blockquote><div><br class=""></div><div>4. I propose to join mbsize and mb2wcisalpha under the single function check_alpha()</div><div>or something, that either accepts char ** and moves it forward, or returns parsed size.</div><div><br class=""></div><div>And why you do not check mbsize() for negative results? In the doc I saw, that it can</div><div>return values like (size_t)-1 or (size_t)-2. And you implementation accepts it as ok.</div><br class=""><blockquote type="cite" class=""><div class=""><div class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>int len = 1;<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>for (c = *++pos; pos < end && (isalpha(c) || c == '_' || isdigit(c));<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span> c = *++pos)<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>for (c = *(pos += mb_size);<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> pos < end && (((mb_size = mbsize(pos, end - pos))<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> && mb2wcisalpha((char *)pos, mb_size)) || c == '_' || isdigit(c));<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span> c = *(pos += mb_size))<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span><span class="Apple-tab-span" style="white-space:pre"> </span>++len;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>assert(len > 0);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>parser->pos = pos;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>node->type = JSON_PATH_STR;<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>node->str = str;<br class="">-<span class="Apple-tab-span" style="white-space:pre"> </span>node->len = len;<br class="">+<span class="Apple-tab-span" style="white-space:pre"> </span>node->len = (int)(pos - str);<br class=""> <span class="Apple-tab-span" style="white-space:pre"> </span>return 0;<br class=""> }<br class=""><br class="">diff --git a/src/lib/json/path.h b/src/lib/json/path.h<br class="">index 6e8db4c..b1028f6 100644<br class="">--- a/src/lib/json/path.h<br class="">+++ b/src/lib/json/path.h<br class="">@@ -33,6 +33,8 @@<br class=""><br class=""> #include <stdbool.h><br class=""> #include <stdint.h><br class="">+#include <string.h><br class="">+#include <malloc.h><br class=""><br class=""> #ifdef __cplusplus<br class=""> extern "C" {<br class="">diff --git a/test/engine/tuple.result b/test/engine/tuple.result<br class="">index 2d7367a..c4b361a 100644<br class="">--- a/test/engine/tuple.result<br class="">+++ b/test/engine/tuple.result<br class="">@@ -611,7 +611,7 @@ s = box.schema.space.create('test', {format = format})<br class=""> pk = s:create_index('pk')<br class=""> ---<br class=""> ...<br class="">-field2 = {1, 2, 3, "4", {5,6,7}, {key="key1", value="value1"}}<br class="">+field2 = {1, 2, 3, "4", {5,6,7}, {key="key1", value="value1", hello中国world = {中国 = 'test'}}}<br class=""></div></div></blockquote><div><br class=""></div><span class="">5. Please, add unit tests on unicode. See commit</span></div><span class="">49df120aaa7592e6475bf6974fe6f225db9234de (Introduce json_path_parser) for examples.<br class=""></span><div><blockquote type="cite" class=""><div class=""><div class=""><br class=""></div></div></blockquote></div><br class=""></div></body></html>