Tarantool development patches archive
 help / color / mirror / Atom feed
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.
> 
> 

  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