On Mon, Mar 7, 2016 at 4:58 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>> - 32-b: add double functions, including double variables
>> - 32-c: remove \setrandom support (advice to use \set + random instead)
> Here is a rebased version after Tom's updates, 33-b & 33-c. I also extended
> the floating point syntax to signed accept signed exponents, and changed the
> regexpr style to match Toms changes.

Having a look at 33-b, this looks pretty good now, but:

// comments are not allowed.  I'd just remove the two you have.

It make no sense to exit(1) and then return 0, so don't do that.  I
might write this code as:

if (pval->type == PGBT_INT)
   return pval->u.ival;

Assert(pval->type == PGBT_DOUBLE);
/* do double stuff */

This would get rid of the internal-error case here altogether in favor
of testing it via an assertion.

I think that coerceToInt() should not exit(1) when an overflow occurs;
instead, I think the function should be declared as bool
coerceToInt(PgBenchValue *pval, int64 *result), and the error return
should be propagated back to the caller, which can then return false
as we do for other error cases.

I think that some of the places you've used coerceToInt() are not
appropriate.  Like, for example:

+                                                       if
(coerceToInt(rval) == 0)
+                                                       {
fprintf(stderr, "division by zero\n");
+                                                               return false;
+                                                       }

Now, if rval is out of range of an integer, that is going to overflow
while trying to see whether it should divide by zero.  Please work a
little harder here and in similar cases.  Maybe add a helper function
checkIntegerEquality(PgBenchValue *, int64).

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to