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

[Alexander Turenko <alexander.turenko@tarantool.org>]

Hi! Sorry, but no. I'll update the patch on the next week.

WBR, Alexander Turenko.

On Sat, Feb 16, 2019 at 12:06:48AM +0300, Vladislav Shpilevoy wrote:
> 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.
> > 
> >