Re: [PATCH v2 6/8] decimal: allow to encode/decode decimals as MsgPack

[Serge Petrenko <sergepetrenko@tarantool.org>]

> 13 авг. 2019 г., в 0:34, Konstantin Osipov <kostja@tarantool.org> написал(а):
> 
> * 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.
>>  */
>> 
>> +#include "msgpuck.h"
> 
> why this include?

For mp_type in msgpack_to_field_type() and field_type_to_msgpack().

> 
> 
> What is this header file standing (mp_user_tpyes.h) for? 
> 
> 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.

> 
>> +++ b/src/lib/core/mpstream.h
>> @@ -32,6 +32,7 @@
>>  */
>> 
>> #include "diag.h"
>> +#include "decimal.h"
>> 
> 
> 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.

> 
> Please, please, C is not PHP, you don't include everything around
> just because it was a bad day.
> 
>> 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>
>> 
>> +#include "lua/decimal.h"
>> +#include "lib/core/decimal.h"
>> +#include "lib/core/mp_decimal.h"
>> +#include "lib/core/mp_user_types.h"
>> +
> 
> This is a dependency hell.
> 
> 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?

> 
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia


--
Serge Petrenko
sergepetrenko@tarantool.org