Hello Michaël,
>> - remove the short macros (although IMO it is a code degradation)
>
> FWIW, I like this suggestion from Robert.
I'm not especially found of macros, my main reserve is that because of the
length of the function names this necessarily creates lines longer than 80
columns or awkward and repeated new lines within expressions, which are
both ugly.
> +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.
Not that simply: The number is the index in the array of functions, not
the enum value, they are currently off by one, and there may be the same
function with different names for some reason some day, so I do not think
it would be a good idea to enforce the order so that they are identical.
Also this minor search is only executed when parsing the script.
> + * - 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.
Ok for a better comment.
> For mod() there is no need to have an error, returning 0 is fine. You can
> actually do it unconditionally when rval == -1.
Ok.
> + setDoubleValue(retval, d < 0.0? -d: d);
> Nitpick: lack of spaces between the question mark.
Ok.
> NONE is used nowhere, but I think that you could use it for an assertion
> check here: [...]
> Just replace the "none" message by Assert(type != PGBT_NONE) for example.
I've added a use of the macro.
> Another remaining item: should support for \setrandom be dropped? As the
> patch is presented this remains intact.
As you know my opinion is "yes", but I have not receive a clear go about
that from a committer and I'm not motivated by removing and then re adding
code to the patch.
If nothing clear is said, I'll do a patch just for that one functions are
added and submit it to the next CF.
--
Fabien.
