gmpxx: bug in declaration of __gmp_set_expr

[Marc Glisse]

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