From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <sergepetrenko@tarantool.org>
Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH v2 6/8] decimal: allow to encode/decode decimals as
 MsgPack
From: Serge Petrenko <sergepetrenko@tarantool.org>
In-Reply-To: <20190812213454.GI32337@atlas>
Date: Tue, 13 Aug 2019 17:01:48 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <A9106CB2-19A2-4135-80F3-0D7026CF9A0F@tarantool.org>
References: <cover.1565263272.git.sergepetrenko@tarantool.org>
 <820cab72f09d48cf2d9e9cba5e0bba9620e32d96.1565263272.git.sergepetrenko@tarantool.org>
 <20190812213454.GI32337@atlas>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>, tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>


> 13 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 0:34, Konstantin Osipov =
<kostja@tarantool.org> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0):=

>=20
> * Serge Petrenko <sergepetrenko@tarantool.org> [19/08/08 15:01]:
>> index d84748b71..89ea26b41 100644
>> --- a/src/lib/core/mp_user_types.h
>> +++ b/src/lib/core/mp_user_types.h
>> @@ -31,6 +31,12 @@
>>  * SUCH DAMAGE.
>>  */
>>=20
>> +#include "msgpuck.h"
>=20
> why this include?

For mp_type in msgpack_to_field_type() and field_type_to_msgpack().

>=20
>=20
> What is this header file standing (mp_user_tpyes.h) for?=20
>=20
> User types are enum field_type. What other user types do you have
> in mind? Perhaps you mean user-defined types? Well, decimal is not
> a user defined type, it's built-in. The fact that it has to use
> mp_ext format type in msgpack is just a coincidence of Tarantool
> using msgpack for its internal data representation.

Ok, we may name the file extension_types or ext_types or mp_ext_types.

>=20
>> +++ b/src/lib/core/mpstream.h
>> @@ -32,6 +32,7 @@
>>  */
>>=20
>> #include "diag.h"
>> +#include "decimal.h"
>>=20
>=20
> why this include?

For decimal_t. Because
decimal_t is a typedef for decNumber, and decNumber is a typedef
for an anonymous struct.

I can patch decNumber so that we have a struct decNumber. Then we
will be able to get away with typedef struct decNumber decimal_t in
such headers.

>=20
> Please, please, C is not PHP, you don't include everything around
> just because it was a bad day.
>=20
>> index 6fc2b8278..2e6b5c163 100644
>> --- a/src/lua/msgpack.c
>> +++ b/src/lua/msgpack.c
>> @@ -41,6 +41,11 @@
>> #include <small/region.h>
>> #include <small/ibuf.h>
>>=20
>> +#include "lua/decimal.h"
>> +#include "lib/core/decimal.h"
>> +#include "lib/core/mp_decimal.h"
>> +#include "lib/core/mp_user_types.h"
>> +
>=20
> This is a dependency hell.
>=20
> Why did you add a separate header for decimal encoding/encoding if
> you have to include *everything* in here anyway? Noone will ever
> be able to understand why you decided to include all of these
> here.

Do you want me to add comments describing why we need each header?

>=20
>=20
>=20
> --=20
> Konstantin Osipov, Moscow, Russia


--
Serge Petrenko
sergepetrenko@tarantool.org