Hello Ildar,
Patch v8 applies cleanly and compiles. Global and local "make check ok".
Doc build ok.
For me "random seed" is what is passed to srandom, so the variable
should rather be named hash_seed because there could also be a random
seed (actually, there is in another parallel patch:-). Moreover, this
seed may or may not be random if set, so calling it "random_seed" is
not desirable.
My intention was to introduce seed variable which potentially could be
used in different contexts, not only for hash functions.
Yes, good point. I'd need it for the pseudo-random permutation function.
I renamed it to 'hash_seed' for now. But what do you think about naming
it simply 'seed' or choosing some other general name?
ISTM "seed" that is prone to being used for something else in a script.
What about ":default_seed", which says it all?
Some minor suggestions:
In "make_func", I'd assert that nargs is positive in the default branch.
The default seed may be created with just one assignment instead of
repeated |= ops. Or not:-)
In evalStandardFunc, I'd suggest to avoid the ? construction and use an
if and a direct setIntValue in both case, removing the "result"
variable, as done in other branches (eg bitwise operators...). Maybe
something like:
if (func == murmur2)
setIntValue(retval, getHashXXX(val, seed));
else if (...)
...
else
Assert(0);
so that it is structurally ready for other hash functions if needed.
Documentation:
In the table, use official names in the description, and add wikipedia
links, something like:
FNV hash ->
<ulink
url="https://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function";>FNV-1a
hash</ulink>
murmur2 hash ->
<ulink url="https://en.wikipedia.org/wiki/MurmurHash";>MurmurHash2 hash</ulink>
In the text:
"Hash functions accepts" -> "Hash functions
<literal>hash</literal>, <......> and <....> accept*NO S*"
"... provided hash_seed value is used" => "... provided the value of
<literal>:hash_seed</literal> is used, which is initialized randomly
unless set by the command-line <literal>-D</literal> option."
ISTM that the Automatic Variable table should be in alphabetical order.
--
Fabien.
