[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml
Alexander Turenko
alexander.turenko at tarantool.org
Sat Feb 16 00:23:54 MSK 2019
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.
> >
> >
More information about the Tarantool-patches
mailing list