Re: [Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 7 comments below.

> diff --git a/src/lib/uuid/mp_uuid.c b/src/lib/uuid/mp_uuid.c
> new file mode 100644
> index 000000000..7acfbc797
> --- /dev/null
> +++ b/src/lib/uuid/mp_uuid.c
> @@ -0,0 +1,98 @@
> +
> +#include "mp_uuid.h"
> +#include "msgpuck.h"
> +#include "mp_extension_types.h"
> +#include "lib/uuid/tt_uuid.h"

1. Lib/, as well as core/, can be omitted. That paths anyway
are in -I.

> +
> +inline uint32_t
> +mp_sizeof_uuid(void)
> +{
> +	return mp_sizeof_ext(UUID_PACKED_LEN);
> +}
> diff --git a/src/lib/uuid/mp_uuid.h b/src/lib/uuid/mp_uuid.h
> new file mode 100644
> index 000000000..430cb96d2
> --- /dev/null
> +++ b/src/lib/uuid/mp_uuid.h
> @@ -0,0 +1,90 @@
> +
> +#include <stdint.h>
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct tt_uuid;
> +
> +#define UUID_PACKED_LEN sizeof(struct tt_uuid)

2. Maybe better make it enum. When you do it via the macros, you
workaround the necessity to make include tt_uuid.h, but the macros
anyway can't be actually used without this include. So it is ok
to add the include here + change that to macros.

> +
> +/**
> + * \brief Return the number of bytes an encoded uuid value takes.
> + */

3. I would better use @ than \. The latter is used in the old code
only.

> +uint32_t
> +mp_sizeof_uuid(void);
> +
> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
> index 73ed3ece6..e4fb0cf43 100644
> --- a/src/lua/msgpack.c
> +++ b/src/lua/msgpack.c
> @@ -43,6 +43,7 @@
>  
>  #include "lua/decimal.h" /* lua_pushdecimal() */
>  #include "lib/core/decimal.h" /* decimal_unpack() */
> +#include "lib/uuid/mp_uuid.h" /* mp_decode_uuid() */

4. lib/ can be omitted. Don't know why they are used in other
includes.

diag.h is the most used header from core/, and we never write
core/diag.h.

> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 54d18ac89..bd6bfb008 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -1286,5 +1296,15 @@ tarantool_lua_utils_init(struct lua_State *L)
>  	assert(CTID_CHAR_PTR != 0);
>  	CTID_CONST_CHAR_PTR = luaL_ctypeid(L, "const char *");
>  	assert(CTID_CONST_CHAR_PTR != 0);
> +	rc = luaL_cdef(L, "struct tt_uuid {"
> +				  "uint32_t time_low;"
> +				  "uint16_t time_mid;"
> +				  "uint16_t time_hi_and_version;"
> +				  "uint8_t clock_seq_hi_and_reserved;"
> +				  "uint8_t clock_seq_low;"
> +				  "uint8_t node[6];"
> +			  "};");

5. It is worth adding assert(rc == 0). Otherwise you can omit 'rc = '
at all.

> +	CTID_UUID = luaL_ctypeid(L, "struct tt_uuid");
> +	assert(CTID_UUID != 0);
>  	return 0;
>  }
> diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
> index af4f2f5d5..411c56f71 100644
> --- a/third_party/lua-yaml/lyaml.cc
> +++ b/third_party/lua-yaml/lyaml.cc
> @@ -50,6 +50,9 @@ extern "C" {
>  } /* extern "C" */
>  #include "lua/utils.h"
>  #include "lib/core/decimal.h"
> +#include "lib/core/tt_static.h"
> +#include "lib/core/mp_extension_types.h" /* MP_DECIMAL, MP_UUID */
> +#include "lib/uuid/tt_uuid.h" /* tt_uuid_to_string(), UUID_STR_LEN */

6. lib/ and core/ can be omitted.

>  #define LUAYAML_TAG_PREFIX "tag:yaml.org,2002:"
>  
> @@ -697,10 +700,18 @@ static int dump_node(struct lua_yaml_dumper *dumper)
>        switch (field.ext_type) {
>        case MP_DECIMAL:
>           str = decimal_to_string(field.decval);
> -	 len = strlen(str);
> -	 break;
> +         len = strlen(str);
> +         break;
> +      case MP_UUID:
> +      {
> +         char *buf = tt_static_buf();
> +         tt_uuid_to_string(field.uuidval, buf);
> +         str = buf;
> +         len = UUID_STR_LEN;
> +         break;
> +      }

7. Consider the diff:

====================
diff --git a/third_party/lua-yaml/lyaml.cc b/third_party/lua-yaml/lyaml.cc
index 411c56f71..29dbe7211 100644
--- a/third_party/lua-yaml/lyaml.cc
+++ b/third_party/lua-yaml/lyaml.cc
@@ -703,13 +703,9 @@ static int dump_node(struct lua_yaml_dumper *dumper)
          len = strlen(str);
          break;
       case MP_UUID:
-      {
-         char *buf = tt_static_buf();
-         tt_uuid_to_string(field.uuidval, buf);
-         str = buf;
+         str = tt_uuid_str(field.uuidval);
          len = UUID_STR_LEN;
          break;
-      }
       default:
          assert(0); /* checked by luaL_checkfield() */
       }
====================