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

+                                       case PGBENCH_MOD:
+                                               if (coerceToInt(&rval) == 0)
"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,
+} 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",
+                       else if (retval->type == PGBT_DOUBLE)
+                               fprintf(stderr, "double %f\n",
+                       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.

Reply via email to