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/ht...
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