From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 30 May 2018 22:15:20 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged Message-ID: <20180530191520.GH18850@atlas> References: <7c30b9483a38bd36425d7e0c87b4d15e8893446e.1524228894.git.v.shpilevoy@tarantool.org> <20180510182210.GB30593@atlas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org, vdavydov.dev@gmail.com List-ID: * Vladislav Shpilevoy [18/05/24 23:53]: > 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 = }]). Please do. We can see if anyone complains and revert the change. I am pushing the patch. Please make this last remaining change on a separate branch, maybe independently of the rest of the patch stack. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov