gmpxx: bug in declaration of __gmp_set_expr
Marc Glisse
marc.glisse at normalesup.org
Fri Aug 8 12:57:02 CEST 2008
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
More information about the gmp-bugs
mailing list