From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id BFC62267CB for ; Tue, 5 Feb 2019 14:36:43 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RLm0mhwIw0lz for ; Tue, 5 Feb 2019 14:36:43 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 068E22678F for ; Tue, 5 Feb 2019 14:36:42 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml References: <15b711cb55d34fff1f010fd6df1aa32248f65735.1548123025.git.alexander.turenko@tarantool.org> <3da6fc29-991d-e475-0777-49eacb764b94@tarantool.org> <20190205032947.oibtqsvinfn3v34e@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: Date: Tue, 5 Feb 2019 22:36:40 +0300 MIME-Version: 1.0 In-Reply-To: <20190205032947.oibtqsvinfn3v34e@tkn_work_nb> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko Cc: tarantool-patches@freelists.org Thanks for the fixes! >>> diff --git a/test/app-tap/console.test.lua b/test/app-tap/console.test.lua >>> index 4f915afd7..68f273234 100755 >>> --- a/test/app-tap/console.test.lua >>> +++ b/test/app-tap/console.test.lua >>> @@ -77,11 +77,24 @@ test:is(yaml.decode(client:read(EOL)), '', "clear delimiter") >>> -- >>> -- gh-3476: yaml.encode encodes 'false' and 'true' incorrectly. >>> +-- gh-3662: yaml.encode encodes booleans with multiline format. >>> +-- gh-3583: yaml.encode encodes null incorrectly. >>> -- >>> -test:is(type(yaml.decode(yaml.encode('false'))), 'string') >>> -test:is(type(yaml.decode(yaml.encode('true'))), 'string') >>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string') >>> -test:is(type(yaml.decode(yaml.encode({a = 'false'})).a), 'string') >> >> 1. Are you sure that this is the correct behaviour? So now it is >> not true that decode(encode(obj)) == obj? Looks utterly confusing. >> Or why else you removed these tests? > > They are work as before, but now superseded with simpler ones. It is not obvious. I see a test yaml.encode(false) == "--- false\n...\n" I see another test: yaml.decode('false') == false But where is this? - yaml.decode("--- false\n...\n") == false And why "false" == "--- false\n...\n" according to the tests in the patch? >> >>> +{ >>> + if (len > 5) >>> + return YAML_NO_MATCH; >>> + if (strcmp(str, "false") == 0 || strcmp(str, "no") == 0) >>> + return YAML_FALSE; >>> + if (strcmp(str, "true") == 0 || strcmp(str, "yes") == 0) >> >> 3. I think, it is redundant to firstly check length and then >> call strcmp, which also calculates length (by checking zero >> byte). We need either use only strcmp(), or use length comparison >> + memcmp. The same below for the null parser. > > Okay, it was the premature optimization. > > The more important thing is that is was incorrect: "fals" gives > YAML_FALSE. > > I had fixed it like so, is it okay? > > ``` > if ((len == 5 && memcmp(str, "false", 5) == 0) || > (len == 2 && memcmp(str, "no", 2) == 0)) { > if (out != NULL) > *out = false; > return 0; > } > ... > ``` Yes, this fix is ok, thanks. > >> >>> + return YAML_TRUE; >>> + return YAML_NO_MATCH; >>> +} >>> + >>> +/** >>> + * Verify whether a string represents a null literal in YAML. >>> + * >>> + * Non-standard: don't match an empty string as null. >>> + */ >>> +static yaml_type >>> +yaml_get_null(const char *str, const size_t len){ >> >> 4. ? >> >> static inline int >> yaml_parse_null(const char *str, size_t len, bool *is_null); > > is_null is redundant here, because a return code contain all needed > information. Agreed except that. Changed. My point was to standardize parser functions so as to always return 0/-1 on success/error, and always return an actual value via an out parameter. But now I see, that -1 here is not really an error, but rather 'not being of a requested type'. So here we do not have even a notion of 'error'. In such a case, please, consider the fixes below. Also, I removed optionality of out parameter in yaml_parse_bool in order to get rid of 'if'. diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc index 5afcace1b..a4e03a8ba 100644 --- a/third_party/lua-yaml/lyaml.cc +++ b/third_party/lua-yaml/lyaml.cc @@ -96,46 +96,43 @@ struct lua_yaml_dumper { /** * Verify whether a string represents a boolean literal in YAML. - * * Non-standard: only subset of YAML 1.1 boolean literals are * treated as boolean values. - * - * Return 0 at success, -1 at error. + * @param str Literal. + * @param len Length of @a str. + * @param[out] out Result boolean value. + * @retval Whether @a str represents a boolean value or not. */ -static inline int -yaml_parse_bool(const char *str, size_t len, bool *out) +static inline bool +yaml_is_bool(const char *str, size_t len, bool *out) { if ((len == 5 && memcmp(str, "false", 5) == 0) || (len == 2 && memcmp(str, "no", 2) == 0)) { - if (out != NULL) - *out = false; - return 0; + *out = false; + return true; } if ((len == 4 && memcmp(str, "true", 4) == 0) || (len == 3 && memcmp(str, "yes", 3) == 0)) { - if (out != NULL) - *out = true; - return 0; + *out = true; + return true; } - return -1; + return false; } /** * Verify whether a string represents a null literal in YAML. - * * Non-standard: don't match an empty string, 'Null' and 'NULL' as * null. - * - * Return 0 at success, -1 at error. + * @retval Whether @a str represents NULL or not. */ -static inline int -yaml_parse_null(const char *str, size_t len) +static inline bool +yaml_is_null(const char *str, size_t len) { if (len == 1 && str[0] == '~') - return 0; + return true; if (len == 4 && memcmp(str, "null", 4) == 0) - return 0; - return -1; + return true; + return false; } static void generate_error_message(struct lua_yaml_loader *loader) { @@ -254,9 +251,9 @@ static void load_scalar(struct lua_yaml_loader *loader) { return; } else if (!strcmp(tag, "bool")) { bool value = false; - int rc = yaml_parse_bool(str, length, &value); + bool rc = yaml_is_bool(str, length, &value); (void) rc; - assert(rc == 0); + assert(rc); lua_pushboolean(loader->L, value); return; } else if (!strcmp(tag, "binary")) { @@ -275,10 +272,10 @@ static void load_scalar(struct lua_yaml_loader *loader) { */ lua_pushliteral(loader->L, ""); return; - } else if (yaml_parse_null(str, length) == 0) { + } else if (yaml_is_null(str, length)) { luaL_pushnull(loader->L); return; - } else if (yaml_parse_bool(str, length, &value) == 0) { + } else if (yaml_is_bool(str, length, &value)) { lua_pushboolean(loader->L, value); return; } @@ -610,6 +607,7 @@ static int dump_node(struct lua_yaml_dumper *dumper) yaml_event_t ev; yaml_scalar_style_t style = YAML_PLAIN_SCALAR_STYLE; int is_binary = 0; + bool unused; char buf[FPCONV_G_FMT_BUFSIZE]; struct luaL_field field; @@ -644,9 +642,8 @@ static int dump_node(struct lua_yaml_dumper *dumper) return dump_table(dumper, &field); case MP_STR: str = lua_tolstring(dumper->L, -1, &len); - if ((yaml_parse_null(str, len) == 0) - || (yaml_parse_bool(str, len, NULL) == 0) - || (lua_isnumber(dumper->L, -1))) { + if (yaml_is_null(str, len) || yaml_is_bool(str, len, &unused) || + lua_isnumber(dumper->L, -1)) { /* * The string is convertible to a null, a boolean or * a number, quote it to preserve its type.