[PATCH] new function and range control in std_vmod

[Poul-Henning Kamp]

--------
In message <CAL6a+5iK+RDmOJi54EWHRejtauvTMm9O+C+63X_GCMCUL+dUBg at mail.gmail.com>
, Arianna Aondio writes:

>  VCL_REAL __match_proto__(td_std_time2real)
> -vmod_time2real(VRT_CTX, VCL_TIME t)
> +vmod_time2real(VRT_CTX, VCL_TIME t, VCL_REAL r)
>  {
>  	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
>  
> +	if (t > DBL_MAX || t < DBL_MIN || (!isfinite(t)))
> +		return  (r);
>  	return (t);
>  }

Ohhh dear...

Have I told you how much I hate floating-point corner cases ?

Don't take this review the wrong way, I'm just using it as an
educational tool to give you guys an idea how horrible FP-work is:


First, if you ran this on a machine with silly ieee default flags and
t were NAN of INF, your program would likely core-dump with a FP-trap.

You should do the isfinite() test first to avoid this.

There is a set of special comparison functions that will explictly not
trap:  isgreater(3), isless(3) etc. etc.

Second, in this particular case (time/real) they're both doubles, so
all you need is the isfinite() test.

Third, DBL_MIN is not, as many suppose the largest negative number
a double can hold, but rather the smallest positive number it can
hold without denormalizing.   Your code above fails on any argument
zero or less.

Negative timestamps are allowed, they're moments in time before 1970.

>  VCL_TIME __match_proto__(td_std_real2time)
> -vmod_real2time(VRT_CTX, VCL_REAL r)
> +vmod_real2time(VRT_CTX, VCL_REAL r, VCL_TIME t)
>  {
>       CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
>
> +     if (r > LONG_MAX || r < LONG_MIN || (!isfinite(r)))
> +             return  (t);
>       return (r);
>  }

Why you use LONG_MIN/LONG_MAX here when VCL_TIME is a double ?

> +VCL_INT __match_proto__(td_std_real2integer)
> +vmod_real2integer(VRT_CTX, VCL_REAL r, VCL_INT i)
> +{
> +	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
> +
> +	if (r > LONG_MAX || r < LONG_MIN || (!isfinite(r)))
> +		return  (i);
> +	return ((long)floor(r));
> +}
> +
>  VCL_TIME __match_proto__(td_std_real2time)

isfinite() should be first.

This one adds a new horror to the beastiarium:  Rounding.

I don't think your choice of floor() is correct, I would expect round().

But we're not done:

Imagine the double number is (LONG_MAX + .4), that should round down
and pass the <= LONG_MAX test.

However, it will not, because C will promote LONG_MAX to a double
making the comparison fail.

You have to explicitly round the double first, which means that
you cannot use the isless() family, since round() would TRAP before
isless got called, so something like:

	if (!isfinite(r))
		return (i);
	r = round(r);
	if (r > LONG_MAX || r < LONG_MIN)
		return (i);
	return ((long)r);

> +		set resp.http.x-fallback1 = std.real2time(382473843278423748923749238429834.848383, now);
> +		set resp.http.x-fallback2 = std.real2integer(382473843278423748923749238429834.848383, 2);

First you should add a comment about what these magic numbers are, and
what limits you expect them to exceed.

Second, they're no good: If LONG were a 128bit type the test would fail.

Third, this might fail on machines with FP implementations than double=ieeebin64

Fourth, passing *precise* FP values around in decimal format is wrong and
imprecise, setting you up for rounding error grief.

There is a special printf(3) format for representing FP numbers exactly,
the "%a" format which looks like "0x1.921fb54411744p+1"

In this case a better strategy might be to use "9e99"

-- 
Poul-Henning Kamp       | UNIX since Zilog Zeus 3.20
phk at FreeBSD.ORG         | TCP/IP since RFC 956
FreeBSD committer       | BSD since 4.3-tahoe    
Never attribute to malice what can adequately be explained by incompetence.