[GiNaC-list] [PATCH] fix bug/feature causing non-deterministic output

[Alexei Sheplyakov]

Hello,

On Wed, May 21, 2014 at 06:17:44PM +0200, Valery Yundin wrote:

> Please see attached patch which fixes rather peculiar "feature" of GiNaC -
> non-determinism.

There's nothing to fix here, move on.

Non-deterministic term order is OK: GiNaC is designed for processing expressions
consisting of (many) millions of terms (expressions of this size are very common
in high energy physics). The intermediate expressions (which typically are
the most sizable) are not going to be printed other than for debugging purposes.
Also the output of GiNaC is mostly processed by other programs rather than
a human users.

Last but not least your patch is just plain buggy.

> diff --git a/ginac/basic.cpp b/ginac/basic.cpp
> index 4287f26..9f982b5 100644
> --- a/ginac/basic.cpp
> +++ b/ginac/basic.cpp
> @@ -781,7 +781,8 @@ return_type_t basic::return_type_tinfo() const
>   *  would all end up with the same hashvalue. */
>  unsigned basic::calchash() const
>  {
> -	unsigned v = make_hash_seed(typeid(*this));
> +	static const unsigned seed = make_hash_seed(typeid(*this));

This is just plain wrong. The hash seed should be computed from the type
information of the *current* object rather than from the type of the object
which initiated the very first call to calchash(). The idea is that object
having a different type are typically different so they should have different
hash values (even if their operands are the same). In theory one can use
a lookup table in order to not recompute the hash seed for the same type
twice (GiNaC uses a different approach - make the hash function fast enough
so caching gives no advantages).

> +	unsigned v = seed;
>  	for (size_t i=0; i<nops(); i++) {


In general any patch which makes the hash function substantially slower for
nothing good at all will be rejected. If someone needs a predictable output,
please write the corresponding print_context.


Best regards,
	Alexei