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

[Konstantin Osipov <kostja@tarantool.org>]

* 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.

>  
> +/**
> + * 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.

> + * @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.

> + * @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).

> + */
> +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?

dumper.end_tag = &dumper.begin_tag + (tag_handle != NULL && tag_prefix != NULL);
> +      dumper.end_tag = &dumper.begin_tag + 1;
> +   else
> +      dumper.end_tag = &dumper.begin_tag;
> +   dumper.cfg = serializer;
>     dumper.error = 0;
>     /* create thread to use for YAML buffer */
>     dumper.outputL = lua_newthread(L);
> @@ -684,11 +712,46 @@ error:
>     }
>  }
>  
> +static int l_dump(lua_State *L) {

Missing newline.

The function needs a formal comment, even though it's trivial.

> +   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.

> + * @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?

Which begs the question, why do you need a new function rather
than extend an existing one with options?

> + */
> +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. 

> +   return lua_yaml_encode_impl(L, serializer, handle, prefix);
> +}
> +
>  static int
>  l_new(lua_State *L);

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov