[tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 24 23:50:34 MSK 2018
Hello. Thanks for the review.
On 10/05/2018 21:22, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [18/04/20 16:25]:
>> index 430f4be2c..8329f84e9 100644
>> --- a/third_party/lua-yaml/lyaml.cc
>> +++ b/third_party/lua-yaml/lyaml.cc
>> @@ -78,6 +78,8 @@ struct lua_yaml_dumper {
>> unsigned int anchor_number;
>> yaml_emitter_t emitter;
>> char error;
>> + yaml_tag_directive_t begin_tag;
>> + yaml_tag_directive_t *end_tag;
>
> Please add comments for the new members.
@@ -78,7 +78,16 @@ struct lua_yaml_dumper {
unsigned int anchor_number;
yaml_emitter_t emitter;
char error;
+ /** Global tag to label the result document by. */
yaml_tag_directive_t begin_tag;
+ /**
+ * - end_tag == &begin_tag - a document is not labeld with a
+ * global tag.
+ * - end_tag == &begin_tag + 1 - a document is labeled with a
+ * global tag specified in begin_tag attribute. End_tag pointer
+ * is used instead of tag count because of libyaml API - it
+ * takes begin and end pointers of tags array.
+ */
yaml_tag_directive_t *end_tag;
>
>>
>> +/**
>> + * Encode a Lua object or objects into YAML documents onto Lua
>> + * stack.
>
> Encode an object or objects on Lua stack into YAML stream.
@@ -622,9 +631,9 @@ static void find_references(struct lua_yaml_dumper *dumper) {
}
/**
- * Encode a Lua object or objects into YAML documents onto Lua
- * stack.
+ * Encode an object or objects on Lua stack into YAML stream.
>
>> + * @param L Lua stack to get arguments and push result.
>> + * @param serializer Lua YAML serializer.
>> + * @param tag_handle NULL, or a global tag handle. Handle becomes
>> + * a synonym for prefix.
>
> The handle becomes a synonym for prefix.
>
>
> I don't understand what this means.
Handle and prefix are YAML standard terms. Consider this tag:
%TAG !push! tag:tarantool.io/push,2018
Here !push! is a handle, tag:tarantool.io/push,2018 - prefix.
When in a document !push! is written, it is exposed to
!tag:tarantool.io/push,2018.
Example:
-- !push!str string_value
Same as:
-- !tag:tarantool.io/push,2018!str string_value
So here handle '!push!' is exposed to 'tag:tarantool.io/push,2018'.
Look:
tarantool> a = yaml.decode('!tag:tarantool.io/push,2018!str string_value')
---
...
tarantool> a
---
- string_value
...
So handle is synonym for prefix.
>
>> + * @param tag_prefix NULL, or a global tag prefix, to which @a
>> + * handle is expanded.
>
> Perhaps you could say a few words here about handles, prefixes and
> expansions, or, better yet, quote the relevant parts of YAML
> standard.
- * @param tag_handle NULL, or a global tag handle. Handle becomes
- * a synonym for prefix.
+ * @param tag_handle NULL, or a global tag handle. For global tags
+ * details see the standard:
+ * http://yaml.org/spec/1.2/spec.html#tag/shorthand/.
* @param tag_prefix NULL, or a global tag prefix, to which @a
- * handle is expanded.
+ * handle is expanded. Example of a tagged document:
+ * handle prefix
+ * ____ ________________________
+ * / \ / \
+ * '%TAG !push! tag:tarantool.io/push,2018
+ * --- value
+ * ...
+ *
>
>> + * @retval nil, error Error.
>> + * @retval not nil Lua string with dumped object.
>
> The return value is integer. What did you mean to say?
> For Lua functions it's better to write something like -2, +1
> (pops two, pushes 1).
- * @retval nil, error Error.
- * @retval not nil Lua string with dumped object.
+ * @retval 2 Pushes two values on error: nil, error description.
+ * @retval 1 Pushes one value on success: string with dumped
+ * object.
*/
>
>> + */
>> +static int
>> +lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer,
>> + const char *tag_handle, const char *tag_prefix)
>> +{
>> struct lua_yaml_dumper dumper;
>> int i, argcount = lua_gettop(L);
>> yaml_event_t ev;
>>
>> dumper.L = L;
>> - dumper.cfg = luaL_checkserializer(L);
>> + dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
>> + dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
>> + /*
>> + * If a tag is specified, then tag list is not empty and
>> + * consists of a single tag.
>> + */
>> + if (tag_handle != NULL && tag_prefix != NULL)
>
> Why do you need to check both?
@@ -661,11 +661,12 @@ lua_yaml_encode_impl(lua_State *L, int argcount,
dumper.L = L;
dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
+ assert((tag_handle == NULL) == (tag_prefix == NULL));
/*
* If a tag is specified, then tag list is not empty and
* consists of a single tag.
*/
- if (tag_handle != NULL && tag_prefix != NULL)
+ if (tag_prefix != NULL)
dumper.end_tag = &dumper.begin_tag + 1;
else
dumper.end_tag = &dumper.begin_tag;
>
> dumper.end_tag = &dumper.begin_tag + (tag_handle != NULL && tag_prefix != NULL);
I very do not like arithmetic operations on boolean and integers, so no.
>> @@ -684,11 +712,46 @@ error:
>> }
>> }
>>
>> +static int l_dump(lua_State *L) {
>
> Missing newline.
Not missing. I did it advisedly. The lyaml.cc file has its own code
style, that slightly differs from Tarantool's. We must either convert
the entire file to Tarantool, or obey lyaml style.
>
> The function needs a formal comment, even though it's trivial.
+/**
+ * Dump Lua objects into YAML string. It takes any argument count,
+ * and dumps each in a separate document.
+ * @retval 1 Pushes one value: string with dumped documents.
+ */
>
>> + return lua_yaml_encode_impl(L, luaL_checkserializer(L), NULL, NULL);
>> +}
>> +
>> +/**
>> + * Dump a Lua object into YAML string with a global TAG specified.
>
> Serialize a Lua object as YAML string, taking into account a
> global tag, if it's supplied in the arguments.
- * Dump a Lua object into YAML string with a global TAG specified.
+ * Serialize a Lua object as YAML string, taking into account a
+ * global tag.
I did not add "if it's supplied in the arguments", because a tag is
required.
>
>> + * @param options First argument on a stack, consists of two
>> + * options: tag prefix and tag handle.
>> + * @param object Lua object to dump under the global tag.
>> + * @retval Lua string with dumped object.
>
> Why do you take options first, object second? AFAICS we usually
> take object first, options second. Let's be consistent?
+ * @param object Lua object to dump under the global tag.
+ * @param options Table with two options: tag prefix and tag
+ * handle.
>
> Which begs the question, why do you need a new function rather
> than extend an existing one with options?
Original yaml.encode has this signature: encode(...). It takes
any argument count, and dumps each. So I can not add an option to
this. Example:
yaml.encode(object, {prefix = ..., handle = ...})
What to do, dump both arguments or use the second as options? I think
that we lost a chance to change encode() signature, it is already public
and documented, and will always take (...) arguments.
But here it is documented
https://tarantool.io/en/doc/1.9/reference/reference_lua/yaml.html?highlight=yaml#lua-function.yaml.encode
that yaml.encode has one argument. Maybe no one noticed that it takes multiple
values. If you want, I could change its signature from (...) to (value, [{tag = <tag>}]).
>
>> + */
>> +static int l_dump_tagged(lua_State *L)
>> +{
>> + struct luaL_serializer *serializer = luaL_checkserializer(L);
>> + int argcount = lua_gettop(L);
>> + if (argcount != 2 || !lua_istable(L, 1)) {
>> +usage_error:
>> + return luaL_error(L, "Usage: encode_tagged({prefix = <string>, "\
>> + "handle = <string>}, object)");
>> + }
>> + lua_getfield(L, 1, "prefix");
>> + if (! lua_isstring(L, -1))
>> + goto usage_error;
>> + const char *prefix = lua_tostring(L, -1);
>> + lua_getfield(L, 1, "handle");
>> + if (! lua_isstring(L, -1))
>> + goto usage_error;
>> + const char *handle = lua_tostring(L, -1);
>> + lua_pop(L, 2);
>> + lua_replace(L, 1);
>> + lua_settop(L, 1);
>
> AFAIR you invalidate handle and prefix pointers as soon as you pop
> and replace things on the stack.
Fixed on the branch.
From the next review I fixed usage tests:
@@ -71,14 +71,13 @@ end
local function test_tagged(test, s)
test:plan(7)
- local must_be_err = 'Usage: encode_tagged(object, {prefix = <string>, handle = <string>})'
local prefix = 'tag:tarantool.io/push,2018'
local ok, err = pcall(s.encode_tagged)
- test:is(err, must_be_err, "encode_tagged usage")
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
ok, err = pcall(s.encode_tagged, 100, {})
- test:is(err, must_be_err, "encode_tagged usage")
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
ok, err = pcall(s.encode_tagged, 200, {handle = true, prefix = 100})
- test:is(err, must_be_err, "encode_tagged usage")
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
local ret
ret, err = s.encode_tagged(300, {handle = '!push', prefix = prefix})
test:is(ret, nil, 'non-usage and non-oom errors do not raise')
See below the entire patch.
=======================================================================
commit 0117eac46095ac7fbf7d36c7ebfeb7fe16f96cc2
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date: Mon Apr 2 23:40:30 2018 +0300
yaml: introduce yaml.encode_tagged
Encode_tagged allows to define one global YAML tag for a
document. Tagged YAML documents are going to be used for
console text pushes to distinguish actual box.session.push() from
console.print(). The first will have tag !push, and the
second - !print.
diff --git a/test/app-tap/yaml.test.lua b/test/app-tap/yaml.test.lua
index def278570..bb75ce4ea 100755
--- a/test/app-tap/yaml.test.lua
+++ b/test/app-tap/yaml.test.lua
@@ -69,9 +69,28 @@ local function test_output(test, s)
"--- |-\n Tutorial -- Header\n ====\n\n Text\n...\n", "tutorial string");
end
+local function test_tagged(test, s)
+ test:plan(7)
+ local prefix = 'tag:tarantool.io/push,2018'
+ local ok, err = pcall(s.encode_tagged)
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+ ok, err = pcall(s.encode_tagged, 100, {})
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+ ok, err = pcall(s.encode_tagged, 200, {handle = true, prefix = 100})
+ test:isnt(err:find('Usage'), nil, "encode_tagged usage")
+ local ret
+ ret, err = s.encode_tagged(300, {handle = '!push', prefix = prefix})
+ test:is(ret, nil, 'non-usage and non-oom errors do not raise')
+ test:is(err, "tag handle must end with '!'", "encode_tagged usage")
+ ret = s.encode_tagged(300, {handle = '!push!', prefix = prefix})
+ test:is(ret, "%TAG !push! "..prefix.."\n--- 300\n...\n", "encode_tagged usage")
+ ret = s.encode_tagged({a = 100, b = 200}, {handle = '!print!', prefix = prefix})
+ test:is(ret, "%TAG !print! tag:tarantool.io/push,2018\n---\na: 100\nb: 200\n...\n", 'encode_tagged usage')
+end
+
tap.test("yaml", function(test)
local serializer = require('yaml')
- test:plan(10)
+ test:plan(11)
test:test("unsigned", common.test_unsigned, serializer)
test:test("signed", common.test_signed, serializer)
test:test("double", common.test_double, serializer)
@@ -82,4 +101,5 @@ tap.test("yaml", function(test)
test:test("ucdata", common.test_ucdata, serializer)
test:test("compact", test_compact, serializer)
test:test("output", test_output, serializer)
+ test:test("tagged", test_tagged, serializer)
end)
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 430f4be2c..ca7f4d224 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -78,6 +78,17 @@ struct lua_yaml_dumper {
unsigned int anchor_number;
yaml_emitter_t emitter;
char error;
+ /** Global tag to label the result document by. */
+ yaml_tag_directive_t begin_tag;
+ /**
+ * - end_tag == &begin_tag - a document is not labeld with a
+ * global tag.
+ * - end_tag == &begin_tag + 1 - a document is labeled with a
+ * global tag specified in begin_tag attribute. End_tag pointer
+ * is used instead of tag count because of libyaml API - it
+ * takes begin and end pointers of tags array.
+ */
+ yaml_tag_directive_t *end_tag;
lua_State *outputL;
luaL_Buffer yamlbuf;
@@ -571,7 +582,8 @@ static int dump_node(struct lua_yaml_dumper *dumper)
static void dump_document(struct lua_yaml_dumper *dumper) {
yaml_event_t ev;
- if (!yaml_document_start_event_initialize(&ev, NULL, NULL, NULL, 0) ||
+ if (!yaml_document_start_event_initialize(&ev, NULL, &dumper->begin_tag,
+ dumper->end_tag, 0) ||
!yaml_emitter_emit(&dumper->emitter, &ev))
return;
@@ -618,13 +630,52 @@ static void find_references(struct lua_yaml_dumper *dumper) {
}
}
-static int l_dump(lua_State *L) {
+/**
+ * Encode an object or objects on Lua stack into YAML stream.
+ * @param L Lua stack to get arguments and push result.
+ * @param serializer Lua YAML serializer.
+ * @param tag_handle NULL, or a global tag handle. For global tags
+ * details see the standard:
+ * http://yaml.org/spec/1.2/spec.html#tag/shorthand/.
+ * @param tag_prefix NULL, or a global tag prefix, to which @a
+ * handle is expanded. Example of a tagged document:
+ * handle prefix
+ * ____ ________________________
+ * / \ / \
+ * '%TAG !push! tag:tarantool.io/push,2018
+ * --- value
+ * ...
+ *
+ * @retval 2 Pushes two values on error: nil, error description.
+ * @retval 1 Pushes one value on success: string with dumped
+ * object.
+ */
+static int
+lua_yaml_encode_impl(lua_State *L, struct luaL_serializer *serializer,
+ const char *tag_handle, const char *tag_prefix)
+{
struct lua_yaml_dumper dumper;
- int i, argcount = lua_gettop(L);
yaml_event_t ev;
+ int argcount;
dumper.L = L;
- dumper.cfg = luaL_checkserializer(L);
+ dumper.begin_tag.handle = (yaml_char_t *) tag_handle;
+ dumper.begin_tag.prefix = (yaml_char_t *) tag_prefix;
+ assert((tag_handle == NULL) == (tag_prefix == NULL));
+ /*
+ * If a tag is specified, then tag list is not empty and
+ * consists of a single tag.
+ */
+ if (tag_prefix != NULL) {
+ dumper.end_tag = &dumper.begin_tag + 1;
+ /* Only one object can be encoded with a tag. */
+ argcount = 1;
+ } else {
+ dumper.end_tag = &dumper.begin_tag;
+ /* When no tags - encode all the stack. */
+ argcount = lua_gettop(L);
+ }
+ dumper.cfg = serializer;
dumper.error = 0;
/* create thread to use for YAML buffer */
dumper.outputL = lua_newthread(L);
@@ -643,7 +694,7 @@ static int l_dump(lua_State *L) {
!yaml_emitter_emit(&dumper.emitter, &ev))
goto error;
- for (i = 0; i < argcount; i++) {
+ for (int i = 0; i < argcount; i++) {
lua_newtable(L);
dumper.anchortable_index = lua_gettop(L);
dumper.anchor_number = 0;
@@ -684,11 +735,49 @@ error:
}
}
+/**
+ * Dump Lua objects into YAML string. It takes any argument count,
+ * and dumps each in a separate document.
+ * @retval 1 Pushes one value: string with dumped documents.
+ */
+static int l_dump(lua_State *L) {
+ return lua_yaml_encode_impl(L, luaL_checkserializer(L), NULL, NULL);
+}
+
+/**
+ * Serialize a Lua object as YAML string, taking into account a
+ * global tag.
+ * @param object Lua object to dump under the global tag.
+ * @param options Table with two options: tag prefix and tag
+ * handle.
+ * @retval 1 Pushes Lua string with dumped object.
+ * @retval 2 Pushes nil and error message.
+ */
+static int l_dump_tagged(lua_State *L)
+{
+ struct luaL_serializer *serializer = luaL_checkserializer(L);
+ if (lua_gettop(L) != 2 || !lua_istable(L, 2)) {
+usage_error:
+ return luaL_error(L, "Usage: encode_tagged(object, {prefix = <string>, "\
+ "handle = <string>})");
+ }
+ lua_getfield(L, 2, "prefix");
+ if (! lua_isstring(L, -1))
+ goto usage_error;
+ const char *prefix = lua_tostring(L, -1);
+ lua_getfield(L, 2, "handle");
+ if (! lua_isstring(L, -1))
+ goto usage_error;
+ const char *handle = lua_tostring(L, -1);
+ return lua_yaml_encode_impl(L, serializer, handle, prefix);
+}
+
static int
l_new(lua_State *L);
static const luaL_Reg yamllib[] = {
{ "encode", l_dump },
+ { "encode_tagged", l_dump_tagged },
{ "decode", l_load },
{ "new", l_new },
{ NULL, NULL}
More information about the Tarantool-patches
mailing list