On Fri, Jan 29, 2016 at 5:16 AM, Fabien COELHO < fabien.coe...@mines-paristech.fr> wrote: > v22 compared to previous:
Thanks for the new patch! > - remove the short macros (although IMO it is a code degradation) FWIW, I like this suggestion from Robert. > - check for INT64_MIN / -1 (although I think it is useless) > - try not to remove/add blanks lines > - let some assert "as is" > - still exit on float to int overflow, see arguments in other mails +make_op(const char *operator, PgBenchExpr *lexpr, PgBenchExpr *rexpr) +{ + return make_func(find_func(operator), + /* beware that the list is reversed in make_func */ + make_elist(rexpr, make_elist(lexpr, NULL))); +} I think that this should use as argument the function ID, aka PGBENCH_ADD or similar instead of find_func() with an operator character. This saves a couple of instructions. + * - nargs: number of arguments (-1 is a special value for min & max) My fault perhaps, it may be better to mention here that -1 means that min and max need at least one argument, and that the number of arguments is not fixed. + case PGBENCH_MOD: + if (coerceToInt(&rval) == 0) { fprintf(stderr, "division by zero\n"); return false; } - *retval = lval % rval; + if (coerceToInt(&lval) == INT64_MIN && coerceToInt(&rval) == -1) + { + fprintf(stderr, "cannot divide INT64_MIN by -1\n"); + return false; + } For mod() there is no need to have an error, returning 0 is fine. You can actually do it unconditionally when rval == -1. + setDoubleValue(retval, d < 0.0? -d: d); Nitpick: lack of spaces between the question mark. +typedef enum +{ + PGBT_NONE, + PGBT_INT, + PGBT_DOUBLE +} PgBenchValueType; NONE is used nowhere, but I think that you could use it for an assertion check here: + if (retval->type == PGBT_INT) + fprintf(stderr, "int " INT64_FORMAT "\n", retval->u.ival); + else if (retval->type == PGBT_DOUBLE) + fprintf(stderr, "double %f\n", retval->u.dval); + else + fprintf(stderr, "none\n"); Just replace the "none" message by Assert(type != PGBT_NONE) for example. Another remaining item: should support for \setrandom be dropped? As the patch is presented this remains intact. -- Michael