From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 E6D4D445320 for ; Tue, 7 Jul 2020 18:35:56 +0300 (MSK) Date: Tue, 7 Jul 2020 15:35:55 +0000 From: Nikita Pettik Message-ID: <20200707153555.GA26461@tarantool.org> References: <21d7145c1929bc4606c56e9a566477f248637ed1.1593096639.git.imeevma@gmail.com> <20200630115056.GA31599@tarantool.org> <20200705142656.GA135859@tarantool.org> <20200706212706.GB22427@tarantool.org> <5bbf04dc-addd-f188-f371-fba854f0a468@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5bbf04dc-addd-f188-f371-fba854f0a468@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v3 2/8] sql: change implicit cast for assignment List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.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 > > > > > 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_* отдельным тикетом? Тикетом, патчем, фолоу-апом - как хочешь.