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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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.