Re: [boost] [review] Multiprecision review scheduled for June 8th - 17th, 2012

Subject: Re: [boost] [review] Multiprecision review scheduled for June 8th - 17th, 2012
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-05-31 01:43:25

Le 29/05/12 23:08, Jeffrey Lee Hellrung, Jr. a écrit :
> Hi all,
>
> The review of the proposed Boost.Multiprecision library authored by John
> Maddock and Christopher Kormanyos has been scheduled for
>
> June 8th - June 17th, 2012
>
> and will be managed by myself.
>
> I hope everyone interested can reserve some time to read through the
> documentation, try the code out, and post a formal review, either during
> the formal review window or before.
>
>
Hi,

glad to see that the library will be reviewed soon.

I have spent some hours reading the documentation. Here are some 
comments and a lot of questions.

* As all the classes are at the multi-precision namespace, why name the 
main class mp_number and not just number?

typedef mp::number<mp::mpfr_float_backend<300> > my_float;

* I think that the fact that operands of different backends can not be 
mixed on the same operation limits some interesting operations:

I would expect the result of unary operator-() always signed? Is this 
operation defined for signed backends?
I would expect the result of binary operator-() always signed? Is this 
operation defined for signed backends? what is the behavior of 
mp_uint128_t(0) - mp_uint128_t(1)?

It would be great if the tutorial could show that it is possible however 
to add a mp_uint128_t and a mp_int256_t, or isn't it possible?
I guess this is possible, but a conversion is needed before adding the 
operands. I don't know if this behavior is not hiding some possible 
optimizations.

I think it should be possible to mix backends without too much 
complexity and that the library could provide the mechanism so that the 
backend developer could tell to the library about how to perform the 
operation and what should be the result.

* Anyway, if the library authors don't want to open to this feature, the 
limitation should be stated more clearly, e.g in the reference 
documentation
"The arguments to these functions must contain at least one of the 
following:

     An mp_number.
     An expression template type derived from mp_number.
"
there is nothing that let think mixing backend is not provided.

* What about replacing the second bool template parameter by an enum 
class expression_template {disabled, enabled}; which will be more 
explicit. That is

   typedef mp::mp_number<mp::mpfr_float_backend<300>, false> my_float;

versus

   typedef mp::mp_number<mp::mpfr_float_backend<300>, 
mp::expression_template::disabled> my_float;

* As I posted in this ML already I think that allocators and precision 
are orthogonal concepts and the library should allow to associate one 
for fixed precision. What about adding a 3rd parameter to state if it  
is fixed or arbitrary precision?

* Why cpp_dec_float doesn't have a template parameter to give the 
integral digits? or as the C++ standard proposal from Lawrence Crow 
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3352.html), 
take the range and resolution as template parameters?

* What about adding Throws specification on the mp_number and backend 
requirements operations documentation?

* Can the user define a backend for fixed int types that needs to manage 
with overflow?

* Why bit_set is a free function?

* I don't see nothing about overflow for cpp_dec_float backend 
operations. I guess it is up to the user to avoid overflow as for 
integers. what would be the result on overflow? Could this be added to 
the documentation?

* can we convert from a cpp_dec_float_100 to a cpp_dec_float_50? if yes, 
which rounding policy is applied? Do you plan to let the user configure 
the rounding policy?
BTW, I see in the reference "Type mp_number is default constructible, 
and both copy constructible and assignable from:  ... Any type that the 
Backend is constructible or assignable from. "
I would expect to have this information in some way on the tutorial. I 
will appreciate also if section "Constructing and Interconverting 
Between Number Types" says something about  convert_to<T> member function.

If not, what about a mp_number_cast function taking as parameter a 
rounding policy?

* Does the cpp_dec_float back end satisfies any of the Optional 
Requirements? The same question for the other backends?

* Is there a difference between implicit and explicit construction?
* On c++11 compilers providing explicit conversion, couldn't the 
convert_to function be replaced by a explicit conversion operator?
* Are implicit conversion possible?

* Do you plan to add constexpr and noexcept to the interface? After 
thinking a little bit I'm wondering if this is this possible when using 
3pp libraries backends that don't provide them?

* Why do you allow the argument of left and right shift operations to be 
signed and throw an exception when negative? Why don't just forbid it 
for signed types?

* Why the "Non-member standard library function support" can be used 
only with floating-point Backend types? Why not with fixed-point types?

* What is the type of boost::multiprecision::number_category<B>::type 
for all the provided backends? Could the specialization for 
boost::multiprecision::number_category<B>::type be added in the 
documentation of each backend? and why not add also B::signed_types, 
B::unsigned_types, B::float_types, B::exponent_type?

* Why have you chosen the following requirements for the backend?
- negate instead of operator-()
- eval_op instead of operator op=()
- eval_convert_to instead of explicit operator T()
- eval_floor instead of floor

Optimization? Is this optimization valid for short types (e.g. up to 4/8 
bytes)?

* As the developer needs to define a class with some constraints to be a 
model of backend, which are the advantages of requiring free functions 
instead of member functions?
* Couldn't these be optional if the backend defines the usual operations?
* Or could the library provide a trivial backend adaptor that requires 
the backend just to provide the usual operations instead of the eval_xxx?
* How the performances of mp_number<this_trivial_adaptor<float>, false> 
will compare with float?

* I don't see in the reference section the relation between files and 
what is provided by them. Could this be added?

* And last, I don't see anything related to rvalue references and move 
semantics. Have you analyzed if its use could improve the performances 
of the library?

Good luck for the review. A really good work.
Vicente