Re: [Tarantool-patches] [PATCH v1 1/1] sql: fix cast of small negative DOUBLE to INTEGER

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thank you for the review! My answers below.

On Mon, Jul 26, 2021 at 09:47:21PM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/changelogs/unreleased/gh-6225-cast-of-small-negative-double-to-int.md b/changelogs/unreleased/gh-6225-cast-of-small-negative-double-to-int.md
> > new file mode 100644
> > index 000000000..9f5d62231
> > --- /dev/null
> > +++ b/changelogs/unreleased/gh-6225-cast-of-small-negative-double-to-int.md
> > @@ -0,0 +1,4 @@
> > +## bugfix/sql
> > +
> > +* Fixed assert on cast of DOUBLE value that greater than -1.0 and less than 0.0
> > +  to INTEGER and UNSIGNED (gh-6255).
> 
> 1. I tried to revert the patch and only got an error, not an assertion.
This is because I rebased this patch above "sql: disallow cast of negative
DOUBLE to UNSIGNED". You can get an assert if you cast to INTEGER instead.

> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index e4ce233e0..6b95e41d3 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -1322,7 +1322,7 @@ mem_get_uint(const struct Mem *mem, uint64_t *u)
> >  	}
> >  	if (mem->type == MEM_TYPE_DOUBLE) {
> >  		double d = mem->u.r;
> > -		if (d >= 0 && d < (double)UINT64_MAX) {
> > +		if (d > -1.0 && d < (double)UINT64_MAX) {
> 
> 2. I see there are 6 changes extending 0 to -1.0, but only 2
> tests. Can you cover all the changed places?
I tried bo find a test, but this is quite problematic:
For double_to_int_precise()/double_to_uint_precise() it actually does not matter
since "(double)(uint64_t)d == d" check return false for any number greater
than -1 and less than 0. I decided to change these functions so the first
codition in IF would be unlinked to the third.

For mem_get_int() it actually matters since it changes values of is_neg.
However, this flag is ignored everywhere, so no test can be provided.

Function mem_get_uint() is actually the only one, which wrongly returns -1 in
case it gets double value that less than 0 and more than -1. However, the only
case when this matters is actually inside of mem_get_uint_unsafe(), which
returns 0 in case mem_get_uint() returns -1. Unexpectedly, this is the value
that should be returned if mem_get_uint() gets a double value less than 0 and
greater than -1. So this error is covered too.