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

Instant reactions on reading though the documentation : 1) Introduction : I would prefer to see e.g. mp::int128_t rather than mp::mp_int128_t, given the stated purpose of the library. 2) cpp_dec_float : Narrowing conversions are truncating. This is a no-no for the usage I have in mind at the moment. 3) Constructing and interconverting :
// There is a need to be careful with integer types though: cpp_int i = 2; // Ooops, this really just multiplies by 3: i *= 3.14;
One of the priorities for my numeric class implementations has always been to protect naive users from equivalent mistakes. What is your evaluation of the design? It is so far beyond anything I have attempted that I cannot usefully comment. The limitations on conversions and mixed precision operations are disappointing but entirely understandable. The ccp_dec_float narrowing conversions being truncating reinforces the my view that the limitations are probably the correct design. What is your evaluation of the implementation? No useful comments. What is your evaluation of the documentation? Good enough to give me confidence to start using the library. I would never expect more than that. What is your evaluation of the potential usefulness of the library? A vital addition to C++ especially for relatively small tasks in commercial software where the Boost backends can be used because performance is not an overriding concern. Did you try to use the library? With what compiler? Did you have any problems? I have started to use the library to embed test code for my special purpose decimal fixed point classes into an application under development. The only issue, using VS2008 /W4 and Boost 1.48, was the need to disable warning 4996. Unfortunately this has been pre-empted by other work. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? More than a quick reading Are you knowledgeable about the problem domain? I have written several numeric classes but all with a narrow scope and protection of naïve users and accuracy as the priority. These were primarily decimal fixed point. Do you think the library should be accepted as a Boost library? Yes

Many thanks for the review,
1) Introduction : I would prefer to see e.g. mp::int128_t rather than mp::mp_int128_t, given the stated purpose of the library.
OK.
2) cpp_dec_float : Narrowing conversions are truncating. This is a no-no for the usage I have in mind at the moment.
I know, it's something we need to at least try and fix.
3) Constructing and interconverting :
// There is a need to be careful with integer types though: cpp_int i = 2; // Ooops, this really just multiplies by 3: i *= 3.14;
One of the priorities for my numeric class implementations has always been to protect naive users from equivalent mistakes.
Nod. We have a request already for these conversions to be explicit, the next step is to try and disallow the operator overloads (enable_if) for narrowing conversions.
I have started to use the library to embed test code for my special purpose decimal fixed point classes into an application under development. The only issue, using VS2008 /W4 and Boost 1.48, was the need to disable warning 4996.
Nod. We only use C++ std lib functions, but MS in their wisdom have "depricated" some of them :-( Thanks again, John.

Thank you for your review, Keith.
1) Introduction : I would prefer to see e.g. mp::int128_t rather than mp::mp_int128_t, given the stated purpose of the library.
Yes, the consensus seems to be that the additional "mp_" part of the class names is redundant. 2) cpp_dec_float : Narrowing conversions are truncating. This is a no-no for the usage I have in mind at the moment. Yes, almost all reviewers requested rounding as an improvement over truncation. This is one of my parts of the library, so i might need to work on it and maybe seek help from others, since I don't know a clever way to round radix-10 to radix-2. <snip> Best regards, Chris.
participants (3)
-
Christopher Kormanyos
-
John Maddock
-
Keith Burton