Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment

[Nikita Pettik <korablev@tarantool.org>]

On 07 Jul 12:29, Mergen Imeev wrote:
> 
> On 07.07.2020 00:27, Nikita Pettik wrote:
> > On 05 Jul 17:26, Mergen Imeev wrote:
> > > Hi! Thank you for the review. My answers and new patch
> > > below. Sorry, I didn't include diff.
> > > > > commit 21d7145c1929bc4606c56e9a566477f248637ed1
> > > > > Author: Mergen Imeev <imeevma@gmail.com>
> > > > > Date:   Wed May 27 13:49:11 2020 +0300
> > > > > 
> > > > > + if (field_mp_plain_type_is_compatible(type, mp_type, true))
> > > > > +   return 0;
> > > > > + return -1;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * Convert the numeric value contained in MEM to double. If the
> > > > > + * is_precise flag is set, the conversion will succeed only if it
> > > > > + * is lossless.
> > > > > + *
> > > > > + * @param mem The MEM that contains the numeric value.
> > > > > + * @param is_precise Flag.
> > > > > + * @retval 0 if the conversion was successful, -1 otherwise.
> > > > > + */
> > > > > +static int
> > > > > +mem_convert_to_double(struct Mem *mem, bool is_precise)
> > > > > +{
> > > > > + if ((mem->flags & (MEM_Int | MEM_UInt)) == 0)
> > > > > +   return -1;
> > > > > + if ((mem->flags & MEM_Int) != 0) {
> > > > > +   int64_t i = mem->u.i;
> > > > > +   double d = (double)i;
> > > > > +   if (!is_precise || i == (int64_t)d)
> > > > > +     mem_set_double(mem, d);
> > > > > +   else
> > > > > +     return -1;
> > > > > + } else {
> > > > > +   uint64_t u = mem->u.u;
> > > > > +   double d = (double)u;
> > > > > +   if (!is_precise || u == (uint64_t)d)
> > > > > +     mem_set_double(mem, d);
> > > > > +   else
> > > > > +     return -1;
> > > > > + }
> > > > > + mem->field_type = FIELD_TYPE_DOUBLE;
> > > > Why not incorparate field_type assingment into mem_set_double()?
> > > > The same concerns other converting functions.
> > > > 
> > > I think this should be done in #4906 issue since it may
> > > lead to not always simple to understand changes.
> > Эм, не очень понял как трактовать твой ответ. Типо тут работает,
> > а если там засунуть, то что-то начинает падать..? 4906 немного про
> > другое: в любом случае у нас в меме будет и field_type и mp_type.
> > К тому же, и то, и это - рефакторинг и функциональных изменений быть
> > не должно.
> Не уверен, что это чистый рефакторинг, т.к. точно помны возникала
> проблема с тем, что MEM_type мог быть не совместимым с field_type
> т.к. field_type не изменился при переписывании мема.
> 
> Стоит сделать это изменение для mem_set_* отдельным тикетом?

Тикетом, патчем, фолоу-апом - как хочешь.