From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Alexander Turenko <alexander.turenko@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml
Date: Sat, 16 Feb 2019 00:06:48 +0300 [thread overview]
Message-ID: <4007f36f-f5a2-71a5-f138-8a353e064c2d@tarantool.org> (raw)
In-Reply-To: <d434d4ff-59d9-3086-032a-155c8c499c53@tarantool.org>
Hi! Any news here?
On 05/02/2019 20:36, Vladislav Shpilevoy wrote:
> 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.
>
>
next prev parent reply other threads:[~2019-02-15 21:06 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-22 2:12 [tarantool-patches] [PATCH v2 0/3] lua-yaml null/boolean fixes Alexander Turenko
2019-01-22 2:12 ` [tarantool-patches] [PATCH v2 1/3] lua-yaml: verify arguments count Alexander Turenko
2019-01-24 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
2019-02-05 3:29 ` Alexander Turenko
2019-02-05 19:36 ` Vladislav Shpilevoy
2019-02-11 13:32 ` Alexander Turenko
2019-02-15 21:28 ` Vladislav Shpilevoy
2019-01-22 2:12 ` [tarantool-patches] [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml Alexander Turenko
2019-01-24 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-24 21:32 ` Vladislav Shpilevoy
2019-02-05 3:29 ` Alexander Turenko
2019-02-05 19:36 ` Vladislav Shpilevoy
2019-02-15 21:06 ` Vladislav Shpilevoy [this message]
2019-02-15 21:23 ` Alexander Turenko
2019-02-18 18:55 ` Alexander Turenko
2019-02-22 15:14 ` Vladislav Shpilevoy
2019-01-22 2:12 ` [tarantool-patches] [PATCH v2 3/3] lua-yaml: treat an empty document/value as null Alexander Turenko
2019-01-24 21:26 ` [tarantool-patches] " Vladislav Shpilevoy
2019-02-05 3:30 ` Alexander Turenko
2019-01-24 21:26 ` [tarantool-patches] Re: [PATCH v2 0/3] lua-yaml null/boolean fixes Vladislav Shpilevoy
2019-02-25 11:27 ` Kirill Yukhin
2019-03-05 16:40 ` Alexander Turenko
2019-03-06 7:21 ` Kirill Yukhin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4007f36f-f5a2-71a5-f138-8a353e064c2d@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=alexander.turenko@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox