From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 621CB4696C6 for ; Fri, 10 Apr 2020 19:56:58 +0300 (MSK) References: <60281b83d48279b708ad173d271e900943ce8e57.1586476073.git.sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 10 Apr 2020 18:56:57 +0200 MIME-Version: 1.0 In-Reply-To: <60281b83d48279b708ad173d271e900943ce8e57.1586476073.git.sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 3/4] box: add MsgPack encoding/decoding for UUID List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko Cc: tarantool-patches@dev.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 > + > +#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() */ } ====================