[tarantool-patches] Re: [PATCH v2 02/10] yaml: introduce yaml.encode_tagged
Konstantin Osipov
kostja at tarantool.org
Wed May 30 22:15:20 MSK 2018
* Vladislav Shpilevoy <v.shpilevoy at 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 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>}]).
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
More information about the Tarantool-patches
mailing list