[tarantool-patches] Re: [PATCH v2 2/3] lua-yaml: fix boolean/null representation in yaml
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Feb 5 22:36:40 MSK 2019
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