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: http://www.postgresql.org/mailpref/pgsql-hackers