Re: [tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged

[Konstantin Osipov <kostja@tarantool.org>]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [18/05/24 23:53]:
> Hello. Thanks for the review.
> 
> On 10/05/2018 21:22, Konstantin Osipov wrote:
> >* Vladislav Shpilevoy <v.shpilevoy@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>}]).

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