Re: [Boost-users] [boost] [review] Multiprecision review (June 8th - 17th, 2012)

Subject: Re: [Boost-users] [boost] [review] Multiprecision review (June 8th - 17th, 2012)
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2012-06-16 05:16:29

Hi, here it is my review which is a summary of the discussion we have 
the past week.

The scope of the library must be stated more clearly, I don't think the 
library is able to manage with fixed-point arithmetic (as least as I 
understand it).
The reasons why mixed arithmetic is not provided must be added in the scope.

The disadvantages to use mp_number instead of the underlying back-end 
when expression templates are not used and the back-end is lightweight.
>     What is your evaluation of the design?
While I was expecting a more open design/interface, I think that the 
current design is good enough for some application.

* Separating the expression template and the specific number 
representations (front-end/back-end) is a good design decision.

* Mixing back-ends
It is unfortunate that the front-end doesn't takes care of mixed 
arithmetic yet.

I would expect the result of unary operator-() always signed. This 
operation is not defined in the front end as it isn't unary operator+().
I would expect the result of binary operator-() always signed.

Adding mp_uint128_t and mp_int128_t and assign it to a mp_int256_t needs 
two explicit conversions. This interface limits some possible 
optimizations.

I think it should be possible to mix back-ends without too much 
complexity at least when expression templates are not used. When ET are 
used, the ET need to take as parameters the back-end of the parameters 
and the result.

If mixing independent backends (that is BE provided by 3pp) is not 
desired, I think that mixing BE of the same "family" (that is provided 
by the same library/developer) is a must on some domain. The backend 
could contain this meta-data in some way (a typedef).

* back-end requirements

It should be clear what are the back-end requirements, which are 
mandatory, which ones are used only if a specific operation of the 
front-end is used and which one are used only if present as an optimization.

Each back-end should document the requirements it satisfies.

* I would expect the library provide a back-end for representing N-bits 
integers with exactly (N/8)+1 bytes or something like that.

* And also an overflow aware back-end for fixed precision integers.

* Allocators and precision are orthogonal concepts and the library 
should allow to associate one for fixed precision. Adding a 3rd 
parameter to state if it  is fixed or arbitrary precision should help in 
this concern.

* Conversions

IIUC convert_to is used to emulate in c++98 compilers C++11 explicit 
conversions. Could the explicit conversion operator be added on 
compilers supporting it?

The front-end should make the differences 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?

* shift operations

An overload of the shift operations with a unsigned should be provided 
(in addition of the signed one) in order to avoid the sign check.

* Uses of bool

The use of bool in template parameters could be improved by the use of 
an enum class which will be more explicit. E.g

enum class expression_template {disabled, enabled};
enum class sign {unsigned, signed};

* Default for ExpressionTemplates parameter

The ExpresionTemplate parameter could be defaulted to e.g.

ExpresionTemplate = expression_template_trait<Backend>::value

Where expression_template_trait could be defined with a defaulted value as

template<typename Backend>
struct expression_template_trait {
   static const expression_template value =
Backend::expression_template_value;
};

Another defaulted value could also be possible, as disabled(or enabled) by
default

template<typename Backend>
struct expression_template_trait {
   static const expression_template value = expression_template::disabled;
};

But I think a default expressed by the backend is a better default.

* noexcep:

The library interface should use the noexcept (BOOST_NOEXCEPT, ...) 
facilities.

* constexpr:

It is unfortunate that the generic mp_number front end can not make use 
contexpr as not all the backends can ensure this.
It will be worth however seen how far we can go.

* literals

The library doesn't provide some kind of literals. I think that the 
mp_number class should provide a way to create literals if the backend 
is able to. For example
to_nonnegative<2884,-4>()|
will produce a nonnegativefixed point constant with a range and 
resolution just sufficient to hold the value 2884*2^-4.

Adding a move constructor from the backend maybe could help.

constexpr mp_number<BE>::mp_number(const BE&&);

>     What is your evaluation of the implementation?
The performances of mp_number<a_trivial_adaptor<float>, false>respect to 
float and mp_number<a_trivial_adaptor<int>, false> and int should be 
given to show the cost of using the generic interface.

Boost.Move should be used to emulate move semantics in compilers don't 
providing it.

>     What is your evaluation of the documentation?

The documentation is quite good, but there are some issues that need to 
be taken in account, in particular:

* The documentation should contain Throws specification on the mp_number 
and backend requirements operations.

* The tutorial should add more examples concerning implicit or explicit 
conversions.

* The reference should state clearly which kind of rounding is done 
while converting between types.

* The reference should contain the type of 
boost::multiprecision::number_category<B>::type for all the provided 
backends,
and why not add also B::signed_types, B::unsigned_types, B::float_types, 
B::exponent_type?

* The reference section must contain the relation between files and what 
is provided by them.

* The documentation must explain how move semantics helps in this domain 
and what the backend needs to do to profit from this optimization.

* The rounding applied when converting must be documented.

* deviations from builtin behavior: The library say that the behavior 
follows as far as possible the behavior of builtin type. The 
documentation should state clearly (maybe in a specific section) the 
specific differences.

>     What is your evaluation of the potential usefulness of the library?
This library provides a uniform interface for multiprecission number 3pp 
libraries adding some optimization using expression templates, which is 
in its own quite useful.
>     Did you try to use the library? With what compiler? Did you have 
> any problems?
I tried it with some compilers and found just some errors with 
clang-2.9. Note that the errors are not present with clang-3.0.
>     How much effort did you put into your evaluation? A glance? A 
> quick reading? In-depth study?
An in depth study of the interface to report what is missing to use it 
for my fixed-point library. I have not really take a look at the 
implementation.
>     Are you knowledgeable about the problem domain?
I don't know too much about floating points. For the time been, I'm only 
interested in fixed point types.
>
> And, most importantly, please explicitly answer the following question:
>
>     Do you think the library should be accepted as a Boost library?
Even if the current state of the library don't allows me to use it for 
my fixed point library I guess that the authors will add the needed 
features before I could provide similar features myself.
Anyway, the library provides enough features that are missing in Boost, 
so YES, this library should be accepted.

>
> Lastly, please consider that John and Christopher have compiled a TODO 
> list [1] based on pre-review comments. Feel free to comment on the 
> priority and necessity of such TODO items, and whether any might be 
> show-stoppers or warrant conditional acceptance of the library.
> [1] 
> http://svn.boost.org/svn/boost/sandbox/big_number/libs/multiprecision/doc/html/boost_multiprecision/map/todo.html
>
>
>
By order of priority:

First all the documentation issues:

* Document the size requirements of fixed precision ints.

Yes. This should be in the documentation before release. A  fixed 
precision integer backend reducing the size to the maximum will be welcome.

* Be a bit clearer on the effects of sign-magnitude representation of 
cpp_int - min == -max etc.

Yes.

* Document cpp_dec_float precision, rounding, and exponent size.

Yes. this will be of course welcome.

* Can we be clearer in the docs that mixed arithmetic doesn't work?

I will add this even on the scope. And I think this is a must feature 
for future versions of the library.

* Document round functions behaviour better (they behave as in C99).

Yes.

* Document limits on size of cpp_dec_float.
Yes.

Next some not too difficult features:

* Can we differentiate between explicit and implicit conversions in the 
mp_number constructor (may need some new type traits)?

This is a must have, as the backend could have different expectations.

* Make fixed precision orthogonal to Allocator type in cpp_int. Possible 
solution - add an additional MaxBits template argument that defaults to 
0 (meaning keep going till no more space/memory).

Yes, this should be taken in account.

* Can the non-expression template operators be further optimised with 
rvalue reference support?

I think this must be analyzed.

* Can any functions - in particular value initialization - be made 
|constexp|?

Unfortunately I think that the generic mp_number class can not take care 
of this completely, but it should where possible.

I don't know enough the domain to have an opinion on the following:
* Add support for fused multiply add (and subtract). GMP mpz_t could use 
this.
* Document std lib function accuracy.
* Make the exponent type for cpp_dec_float a templare parameter, maybe 
include support for big-integer exponents. Open question - what should 
be the default - int32_t or int64_t?
* Can ring types (exact floating point types) be supported? The answer 
should be yes, but someone needs to write it.
* Should there be a choice of rounding mode (probably MPFR specific)?

Good luck,
Vicente