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 nonnegative
fixed 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.
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