From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladislav Shpilevoy Subject: Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged References: <7c30b9483a38bd36425d7e0c87b4d15e8893446e.1524228894.git.v.shpilevoy@tarantool.org> <20180510182210.GB30593@atlas> Message-ID: Date: Thu, 24 May 2018 23:50:34 +0300 MIME-Version: 1.0 In-Reply-To: <20180510182210.GB30593@atlas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, Konstantin Osipov Cc: vdavydov.dev@gmail.com List-ID: Hello. Thanks for the review. On 10/05/2018 21:22, Konstantin Osipov wrote: > * Vladislav Shpilevoy [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 = }]). > >> + */ >> +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 = , "\ >> + "handle = }, 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 = , handle = })' 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 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 = , "\ + "handle = })"); + } + 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}