gmpxx: bug in declaration of __gmp_set_expr
Marc Glisse
marc.glisse at normalesup.org
Tue Aug 12 22:02:39 CEST 2008
On Tue, 12 Aug 2008, Torbjorn Granlund wrote:
> After some reading on function specialization (which apparently exists
> but only for full specialization), it seems like the smallest change
> would be to replace the old declarations:
>
> 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> &);
>
> by the following matrix:
>
> template <class T> void __gmp_set_expr(mpz_ptr, const __gmp_expr<mpz_t, T> &);
> template <class T> void __gmp_set_expr(mpz_ptr, const __gmp_expr<mpq_t, T> &);
> template <class T> void __gmp_set_expr(mpz_ptr, const __gmp_expr<mpf_t, T> &);
> template <class T> void __gmp_set_expr(mpq_ptr, const __gmp_expr<mpz_t, T> &);
> template <class T> void __gmp_set_expr(mpq_ptr, const __gmp_expr<mpq_t, T> &);
> template <class T> void __gmp_set_expr(mpq_ptr, const __gmp_expr<mpf_t, T> &);
> template <class T> void __gmp_set_expr(mpf_ptr, const __gmp_expr<mpz_t, T> &);
> template <class T> void __gmp_set_expr(mpf_ptr, const __gmp_expr<mpq_t, T> &);
> template <class T> void __gmp_set_expr(mpf_ptr, const __gmp_expr<mpf_t, T> &);
>
> In a slightly longer term, the __gmp_set_expr(mp*_t, const mp*_class&)
> should IMHO become overloads instead of specializations, but it does not
> really matter.
>
> A simple way to detect that there is something wrong:
>
> mpz_class a=1;
> mpz_t z; mpz_init(z);
> __gmp_set_expr(z,a+a); // works
> __gmp_set_expr(z,a); // currently fails
>
> I am too C++ ignorant to understand this.
>
> Your example seem somewhat strange, since it accesses __gmp_set_expr
> which surely is meant to be internal.
I agree it is likely meant as internal, but this is the only way to
demonstrate the problem without patching gmpxx.h. An other way to show
what is going on is to put a different printf (or std::cout<<...) in each
definition of __gmp_set_expr, but that requires modifying gmpxx.h. If we
do this and run the testsuite, we can see that the versions with a
mp*_class as argument are never called (at least if we use g++).
> For suggested C++ changes, a patch and a change log and an explanation
> of what problem is solved, or what improvements result, would be the
> preferred form of contribution...
Patch: I guess I presented it in a form that is close enough to a patch (3
declarations out, 9 in, at the same place).
Changelog:
* gmpxx.h (__gmp_set_expr): make the declarations match the
definitions, so they are not considered as overloads instead.
Problem: (prepare aspirin or something equivalent if you want to read)
We have currently:
template <class T, class U>
void __gmp_set_expr(mpz_ptr, const __gmp_expr<T, U> &); // V1
template <>
void __gmp_set_expr(mpz_ptr z, const __gmp_expr<mpz_t,mpz_t> &w){...} // V2
template <class T>
void __gmp_set_expr(mpz_ptr z, const __gmp_expr<mpz_t, T> &expr){...} // V3
The issue is that V2 can never be called, the following explains why.
V1 and V3 are two overloads, ie two different functions with the same
name. V2 is a specialization, ie the same function with just a different
implementation in this special case. Since V3 is not declared yet, V2 is a
specialization of V1. If we put V2 last, it will be a specialization of V3
(because it selects the version that is closest to it). Someone who knows
how classes work but not functions might believe that V3 is a partial
specialization of V1, but partial specialization does not exist for
functions (only full specializations do, that is there cannot remain any
template argument), so it is considered as a different function (that
overloads the name __gmp_set_expr).
Now when we use __gmp_set_expr, the compiler needs to select which one of
the overloads should be considered. For this, it takes the overload whose
declaration most closely matches the call.
Consider a call __gmp_set_expr(z,a) where z is a mpz_t and a a mpz_class.
The compiler choses an overload between V1 and V3 (V2 is not an overload,
just a specialization) and selects V3 because it most closely matches. For
V2 to be used, the compiler would need to select V1, and once it has
chosen V1, it uses a specialization if there is one that matches or the
general code otherwise. But the only case where V2 matches is a case where
we just saw that V3 was selected over V1, so V2 is never called.
So even when we call __gmp_set_expr on a mpz_class, the generic
__gmp_expr<mpz_t,T> code is used. It means that useless copies tend to be
made, trying to evaluate an expression that is already a number. It also
means that we can't rely on expr having an eval method (which only
compound expressions have). It means there is some dead code. It (very)
luckily does not cause any compilation failure (though google finds some
related issues on windows a few years ago).
With the proposed patch, V1 is replaced by an early declaration of V3, so
V2 is now a specialization of V1==V3, and all problems should disappear.
Note that it would be more natural to put the definition V3 before V2.
This way V2 becomes an overload of V3 even without modifying V1. The fact
that V2 is used in the current version also means (I think) that the early
(I think the term is "forward") declarations like V1 are useless (assuming
of course that V2 comes after V3).
An other possible fix is to make V2 an overload instead of a
specialization, which means it is considered in the overload resolution
and selected over V3 for a call with mpz_class. In practice this means
removing the "template<>" in front of it. V1 can also disappear in this
case.
Although my prefered solution is the last one I mentionned (remove V1 and
all the empty "template<>" before __gmp_set_expr), I proposed what I saw
as a more minor change. I am equally happy to go with any of the two
versions and can prepare a patch corresponding to the "replace
specializations by overloads" fix if necessary.
> I hope to read up on C++ in the not too distant future.
This particular corner of C++ (function specialization) is one of the most
useless parts of the language in my opinion. I'd like to see one case
where overloading is not better.
--
Marc Glisse
More information about the gmp-bugs
mailing list