From: Serge Petrenko <sergepetrenko@tarantool.org> To: Vladimir Davydov <vdavydov.dev@gmail.com> Cc: Georgy Kirichenko <georgy@tarantool.org>, Konstantin Osipov <kostja@tarantool.org>, tarantool-patches@freelists.org Subject: Re: [tarantool-patches] Re: [PATCH v3] Add MsgPack ext types handling. Date: Tue, 4 Jun 2019 18:24:53 +0300 [thread overview] Message-ID: <7C50C83F-5F11-494F-AD18-FC07465FC5DA@tarantool.org> (raw) In-Reply-To: <20190604141153.63z7yefi2uituyw3@esperanza> > 4 июня 2019 г., в 17:11, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а): > > On Mon, Jun 03, 2019 at 04:14:25PM +0300, Serge Petrenko wrote: >> diff --git a/msgpuck.c b/msgpuck.c >> index d0ffb83..e5f7e25 100644 >> --- a/msgpuck.c >> +++ b/msgpuck.c >> @@ -33,6 +33,17 @@ >> #define MP_LIBRARY 1 >> #include "msgpuck.h" >> >> +const char * >> +mp_ext_type_str(enum mp_ext_type type) >> +{ >> + switch(type) { >> + case MP_EXT_TIMESTAMP: >> + return "timestamp"; >> + default: >> + return "undefined"; >> + } >> +} >> + >> size_t >> mp_vformat(char *data, size_t data_size, const char *format, va_list vl) >> { >> @@ -298,9 +309,14 @@ next: \ >> PRINTF("%lg", mp_decode_double(&data)); \ >> break; \ >> case MP_EXT: \ >> - mp_next(&data); \ >> - PRINTF("undefined"); \ >> + { \ >> + int8_t type; \ >> + uint32_t len; \ >> + len = mp_decode_extl(&data, &type); \ >> + PRINTF("%s", mp_ext_type_str(type)); \ >> + data += len; \ >> break; \ >> + } \ > > It isn't exactly helpful to see "timestamp" instead of a timestamp > value. If we add MP_EXT_TIMESTAMP to the library, we should implement > all methods needed to work with it: > > mp_sizeof_timestamp > mp_encode_timestamp > mp_decode_timestamp > mp_vformat > mp_snprint > > Since this would require a lot of effort and is out of the scope of this > issue, I suggest dropping MP_EXT_TIMESTAMP altogether for now. Let's > just simply add mp_ext infrastructure, like you did in this patch. In > tests, you can pass an arbitrary number for MP_EXT type. > >> +/** >> + * \brief Encode extension data of length \a len. >> + * The function is equivalent to mp_encode_extl() + memcpy. >> + * \param data - a buffer >> + * \param ext_data - a pointer to extension data >> + * \param type - extension type to encode >> + * \param len - a extension data length >> + * \return \a data + mp_sizeof_ext(\a len) == >> + * data + mp_sizeof_extl(\a len) + \a len >> + * \sa mp_encode_strl >> + */ >> +MP_PROTO char * >> +mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len); > > Nit: > > - IMO 'type' should go before 'ext_data' in the argument list as it > defines what 'ext_data' is about. > > - I'd rename 'ext_data' argument to 'str' for consistency with > mp_encode_bin/str. Besides, it's shorter. Not really important > so this one is up to you. > > Other than that, the patch looks good to me. Thank you! All fixed. After some consideration, ’str’ is probably better than ‘ext_data’. The diff’s below diff --git a/msgpuck.c b/msgpuck.c index e5f7e25..637755c 100644 --- a/msgpuck.c +++ b/msgpuck.c @@ -33,17 +33,6 @@ #define MP_LIBRARY 1 #include "msgpuck.h" -const char * -mp_ext_type_str(enum mp_ext_type type) -{ - switch(type) { - case MP_EXT_TIMESTAMP: - return "timestamp"; - default: - return "undefined"; - } -} - size_t mp_vformat(char *data, size_t data_size, const char *format, va_list vl) { @@ -313,7 +302,7 @@ next: \ int8_t type; \ uint32_t len; \ len = mp_decode_extl(&data, &type); \ - PRINTF("%s", mp_ext_type_str(type)); \ + PRINTF("undefined"); \ data += len; \ break; \ } \ diff --git a/msgpuck.h b/msgpuck.h index 12ae9c1..7cf59d2 100644 --- a/msgpuck.h +++ b/msgpuck.h @@ -382,13 +382,6 @@ enum mp_type { MP_EXT }; -/** - * \brief MsgPack extension data types. - */ -enum mp_ext_type { - MP_EXT_TIMESTAMP = -1 -}; - /** * \brief Determine MsgPack type by a first byte \a c of encoded data. * @@ -569,15 +562,15 @@ mp_encode_extl(char *data, int8_t type, uint32_t len); * \brief Encode extension data of length \a len. * The function is equivalent to mp_encode_extl() + memcpy. * \param data - a buffer - * \param ext_data - a pointer to extension data * \param type - extension type to encode + * \param str - a pointer to extension data * \param len - a extension data length * \return \a data + mp_sizeof_ext(\a len) == * data + mp_sizeof_extl(\a len) + \a len * \sa mp_encode_strl */ MP_PROTO char * -mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len); +mp_encode_ext(char *data, int8_t type, char *str, uint32_t len); /** * \brief Check that \a cur buffer has enough bytes to decode an ext header. @@ -1584,10 +1577,10 @@ mp_encode_extl(char *data, int8_t type, uint32_t len) } MP_IMPL char * -mp_encode_ext(char *data, char *ext_data, int8_t type, uint32_t len) +mp_encode_ext(char *data, int8_t type, char *str, uint32_t len) { data = mp_encode_extl(data, type, len); - memcpy(data, ext_data, len); + memcpy(data, str, len); return data + len; } diff --git a/test/msgpuck.c b/test/msgpuck.c index c2aa70f..ecc2b26 100644 --- a/test/msgpuck.c +++ b/test/msgpuck.c @@ -82,7 +82,7 @@ static char *data = buf + 1; /* use unaligned address to fail early */ s1[i] = 'a' + i % 26; \ } \ _ext(int8_t ext_type = 0); \ - const char *d1 = mp_encode_##_type(data, s1, _ext(ext_type COMMA) _vl);\ + const char *d1 = mp_encode_##_type(data, _ext(ext_type COMMA) s1, _vl);\ const char *d2; \ uint32_t len2; \ d2 = data; \ @@ -884,7 +884,7 @@ test_mp_print() d = mp_encode_uint(d, 100); d = mp_encode_uint(d, 500); /* let's pack zero-length ext */ - d = mp_encode_extl(d, MP_EXT_TIMESTAMP, 0); + d = mp_encode_extl(d, 0, 0); char bin[] = "\x12test\x34\b\t\n\"bla\\-bla\"\f\r"; d = mp_encode_bin(d, bin, sizeof(bin)); d = mp_encode_map(d, 0); @@ -893,7 +893,7 @@ test_mp_print() const char *expected = "[-5, 42, \"kill bill\", [], " "{\"bool true\": true, \"bool false\": false, \"null\": null, " - "\"float\": 3.14, \"double\": 3.14, 100: 500}, timestamp, " + "\"float\": 3.14, \"double\": 3.14, 100: 500}, undefined, " "\"\\u0012test4\\b\\t\\n\\\"bla\\\\-bla\\\"\\f\\r\\u0000\", {}]"; int esize = strlen(expected);
next prev parent reply other threads:[~2019-06-04 15:24 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-03 13:14 Serge Petrenko 2019-06-04 14:11 ` Vladimir Davydov 2019-06-04 15:24 ` Serge Petrenko [this message] 2019-06-04 15:40 ` [tarantool-patches] " Vladimir Davydov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=7C50C83F-5F11-494F-AD18-FC07465FC5DA@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=georgy@tarantool.org \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [tarantool-patches] Re: [PATCH v3] Add MsgPack ext types handling.' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox