gmpxx: bug in declaration of __gmp_set_expr

[Marc Glisse]

Hello,

trying some modifications on gmpxx, I noticed an issue with the way 
__gmp_set_expr is declared/defined (though it does not seem to cause any 
problem except some extra copies so it can wait until 4.3). The goal of 
this function is that __gmp_set_expr(T,__gmp_expr<U,V>) evaluates the 
expression on the right and uses the result to initialize the T on the 
left. Currently, it looks like this:

template <class T, class U>
void __gmp_set_expr(mpz_ptr, const __gmp_expr<T, U> &);
template <class T, class U>
void __gmp_set_expr(mpq_ptr, const __gmp_expr<T, U> &);
template <class T, class U>
void __gmp_set_expr(mpf_ptr, const __gmp_expr<T, U> &);

... use of __gmp_set_expr (only in template context?) ...

template <>
inline void __gmp_set_expr(mpz_ptr z, const mpz_class &w)
{
   mpz_set(z, w.get_mpz_t());
}

template <class T>
inline void __gmp_set_expr(mpz_ptr z, const __gmp_expr<mpz_t, T> &expr)
{
   expr.eval(z);
}

etc with all possible combinations of mpz/mpq/mpf and 
mp*_class/__gmp_expr.

First, one can notice in the rest of the code that __gmp_set_expr is 
mostly called on compound expressions, and can only be called on mp*_class 
if it is not the same as the first argument (mpz_t,mpz_class is never 
called, but mpz_t,mpq_class can be). This means three of these functions 
are useless, but they don't hurt and might become useful again soon. Now 
if we actually call __gmp_set_expr(mpz_t,mpq_class), we notice that it 
does not call the version of the function that we would expect, but the 
generic __gmp_set_expr(mpz_t,__gmp_expr(mpq_t,T)). The reason appears to 
be the empty "template<>" at the beginning. It looks like a confusion 
between functions (which get overloads) and classes (which get partial 
specializations). Removing the "template<>" seems to solve the issue. But 
then it is surprising the compiler didn't print a warning if writing it 
was wrong. Also, the early declarations are apparently useless (but if we 
want them we should probably declare all overloads).

It would be nice if someone familiar with C++ could comment on this...





By the way I am attaching a version of gmpxx.h where I played with various 
bits:
- small temporaries on stack (I skipped hypot)
- no need to write the code 4 times for all comparison operators
- do some unary operations in-place (except for sqrt for which it looks 
slower)
I am not too happy with the commute thing.
Don't take any of it as-is, it is just there as a list of ideas, even 
though I made sure it still passed the testsuite. I am also attaching a 
patch to mpz/aors to make reallocation less likely, but I am not sure it 
really improves anything with a usual malloc implementation where 
realloc(size+4) is usually cheap.


In gmpxx, it sometimes happens that mpq_set(r,s) is called with r==s in 
addition and substraction (for instance for q+=3), but I noticed that g++ 
-O1 is able to detect it with __builtin_constant_p. We could replace 
mpq_set by if(r!=s)mpqset, but if we decide there is too much overhead 
this way, we can still do it at compile time only with gcc.

There are actually quite a few things (even in the C version) that could 
benefit from the use of gcc's __builtin_constant_p. mul_ui could check 
whether its argument is a power of 2 and call mul_2exp instead. hypot with 
a ui argument could check whether its ui argument is small enough to 
compute its square (computing hypot(x,1) looks like it could happen quite 
naturally). And even stupid tests (add(0), mul(0 or 1) etc) could be made, 
that are useless for hand-written calls to gmp, but can be inconvenient to 
avoid when going through dozens of inline functions. I did not experiment 
with making mpz_set_ui inline to see whether gcc could detect at compile 
time a mpz_t that represents 0 or 1 in order to simplify operations that 
involve those.

Oh and gmpxx seems to be missing a pow function.

-- 
Marc Glisse
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpz_aors.patch
Type: text/x-diff
Size: 2674 bytes
Desc: 
Url : http://gmplib.org/list-archives/gmp-bugs/attachments/20080808/7373e621/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: gmpxx.h.gz
Type: application/octet-stream
Size: 11985 bytes
Desc: 
Url : http://gmplib.org/list-archives/gmp-bugs/attachments/20080808/7373e621/attachment.obj