[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml

[Vladislav Shpilevoy]

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.
> 
>