Even more questionable feature of the C++ interface (GMP 4.0.1)
I did it again and this time is even worse. Without looking at the documentation I thought there should be an assignment operator for assigning an mpz_t to a class_mpz. So I did it, the compiler did not complain even with -Wall -W and then strange behavior started. The guilty is now the following line in <gmpxx.h>
__gmp_expr & operator=(bool b) { mpz_set_ui(mp, b); return *this; }
Strictly speaking, I am not reporting anything new with respect to my previous message. What makes things worse is that I now realize that it is much easier to be bitten by this problem, since, IMHO, the user is somewhat legitimate to assume that mpz_t objects can be used to initialize/assign to mpz_class objects.
My revised proposal is that these two lines in <gmpxx.hh>
__gmp_expr(bool b) { mpz_init_set_ui(mp, b); } __gmp_expr & operator=(bool b) { mpz_set_ui(mp, b); return *this; }
are _replaced_ by their analagous taking an mpz_t argument.
Here is a small test program that exemplifies the problem:
/////////////////////////////////////////////////////////////////////////// #include <gmp.h> #include <gmpxx.h> #include <iostream>
using namespace std;
int main() { mpz_class n; mpz_t m; mpz_init_set_si(m, 123);
// I thought I was assigning 123 to n... n = m; // ... but I was assigning 1! cout << n << endl; } ///////////////////////////////////////////////////////////////////////////
All the best
Roberto
I'm not sure why there's a constructor but not an assignment for an mpz_t. There's probably a good reason. Gerardo?
But yes, in any case we should drop the bool forms to avoid such subtle problems.
Roberto Bagnara bagnara@cs.unipr.it writes:
I did it again and this time is even worse. Without looking at the documentation I thought there should be an assignment operator for assigning an mpz_t to a class_mpz. So I did it, the compiler did not complain even with -Wall -W and then strange behavior started. The guilty is now the following line in <gmpxx.h>
__gmp_expr & operator=(bool b) { mpz_set_ui(mp, b); return *this; }Strictly speaking, I am not reporting anything new with respect to my previous message. What makes things worse is that I now realize that it is much easier to be bitten by this problem, since, IMHO, the user is somewhat legitimate to assume that mpz_t objects can be used to initialize/assign to mpz_class objects.
My revised proposal is that these two lines in <gmpxx.hh>
__gmp_expr(bool b) { mpz_init_set_ui(mp, b); } __gmp_expr & operator=(bool b) { mpz_set_ui(mp, b); return *this; }are _replaced_ by their analagous taking an mpz_t argument.
Here is a small test program that exemplifies the problem:
/////////////////////////////////////////////////////////////////////////// #include <gmp.h> #include <gmpxx.h> #include <iostream>
using namespace std;
int main() { mpz_class n; mpz_t m; mpz_init_set_si(m, 123);
// I thought I was assigning 123 to n... n = m; // ... but I was assigning 1! cout << n << endl; } ///////////////////////////////////////////////////////////////////////////
All the best
Roberto-- Prof. Roberto Bagnara Computer Science Group Department of Mathematics, University of Parma, Italy http://www.cs.unipr.it/~bagnara/ mailto:bagnara@cs.unipr.it
On 2002.04.02 00:45 Kevin Ryde wrote:
I'm not sure why there's a constructor but not an assignment for an mpz_t. There's probably a good reason. Gerardo?
But yes, in any case we should drop the bool forms to avoid such subtle problems.
My revised proposal is that these two lines in <gmpxx.hh>
__gmp_expr(bool b) { mpz_init_set_ui(mp, b); } __gmp_expr & operator=(bool b) { mpz_set_ui(mp, b); return*this; }
are _replaced_ by their analagous taking an mpz_t argument.
Sorry for my slow response...
I didn't know about automatic conversion from pointers to bool type. I guess it's for fast testing whether a pointer is null. In view of that, I agree now that construction and assignment from bool must be removed. They're just too dangerous.
Instead, I'm not convinced that assignment from mpz_srcptr should be provided. If so, then one could expect other operators as well -- for example, mpz_class + mpz_t, and so on. I don't think we should want too much interoperability between mpz_class and mpz_t. In my opinion, the right way is "if you use C++, then use mpz_class, not mpz_t" except for backward compatibility or using GMP functions not (yet) supported by the C++ interface. And even in that case, the correct way should be
mpz_class z, w; ... mpz_something(z.get_mpz_t(), w.get_mpz_t());
rather than
mpz_class z; mpz_t w, v; ... mpz_something(w, v); z = w;
Therefore, my approach has been that one should never operate on mpz_t without _explicitly_ promoting it to mpz_class. That's why I defined an explicit constructor as the _only_ way to do the conversion.
Given that, we might not want to define a constructor either, replacing it with an mpz_class::set_mpz_t(mpz_srcptr) function. However, I'm not willing to give up the commodity of being able to write things like
z = mpz_class(w) + 1; // z is mpz_class, w is mpz_t
In attachment there's a version of gmpxx.h where I removed all constructors and assignments from bool -- but I didn't add their mpz_t counterparts. In passing, I've also removed mpf_class::set_str2() (I think now that mpf_class::set_str() is good enough), and thus the need to #include the huge <iostream> (<iosfwd> is enough) and <strstream>.
Gerardo
Ballabio Gerardo - Dip. di Scienza dei Materiali wrote:
I didn't know about automatic conversion from pointers to bool type. I guess it's for fast testing whether a pointer is null. In view of that, I agree now that construction and assignment from bool must be removed. They're just too dangerous.
Instead, I'm not convinced that assignment from mpz_srcptr should be provided. If so, then one could expect other operators as well -- for example, mpz_class + mpz_t, and so on.
...
In attachment there's a version of gmpxx.h where I removed all constructors and assignments from bool -- but I didn't add their mpz_t counterparts. In passing, I've also removed mpf_class::set_str2() (I think now that mpf_class::set_str() is good enough), and thus the need to #include the huge <iostream> (<iosfwd> is enough) and <strstream>.
Hi Gerardo,
all in all, I agree with you. By the way, I have checked that your revised gmpxx.hh is not causing regressions with our C++ code. Thanks a lot
Roberto
participants (3)
-
gerardo.ballabio＠unimib.it -
Kevin Ryde -
Roberto Bagnara