RE: [boost] Formal Review: fixed-point decimal library

From: Paul A. Bristow (boost_at_[hidden])
Date: 2003-07-17 08:22:43

| -----Original Message-----
| From: boost-bounces_at_[hidden]
| [mailto:boost-bounces_at_[hidden]]On Behalf Of Jens Maurer
| Sent: Wednesday, July 09, 2003 5:07 PM
| To: boost_at_[hidden]; boost-announce_at_[hidden]
| Subject: [boost] Formal Review: fixed-point decimal library

Overall I vote to accept this in the Boost library. It appears to meet the needs
of both COBOL programmers and Bean Counters :-)

More detailed and more expert comments have been received, but from a brief
perusal of the documentation and a build of the trydec test program using MSVC
7.1, I have a few minor observations and comments:

1  There are some warnings using 'strict' which are probably cast avoidable:

fixed_decimal.hpp(442) : warning C4389: '==' : signed/unsigned mismatch
fixed_decimal.hpp(446) : warning C4018: '<' : signed/unsigned mismatch
fixed_decimal.hpp(450) : warning C4018: '>' : signed/unsigned mismatch

2  Using namespace std; is needed for more than cout. Also namespace fixdec
would be more helpfully replaced by "using numeric::decimal;"  This makes it
obvious what is being demonstrated.

3  The sample program output could helpfully be added and made more obvious, for
example by changing to output:

numeric_limits<decimal>::is_specialized is true
numeric_limits<decimal>::radix is 10
numeric_limits<decimal>::digits 9
numeric_limits<decimal>::digits10 9
numeric_limits<decimal>::epsilon()  0.000000001

and

        cout << setprecision(2) << showbase << dp // 2052.00
                << "  " << showmoney << dp //$2,052.00
                << "  " << showintl << dp << "\n" //  USD2,052.00
                << noshowmoney << noshowintl << dn // -1,026.00
                << "  " << showmoney << dn << "  " // -$1,026.00
                << showintl << dn << "\n"; //  USD-1,026.00

And adding the complete output as a comment is very helpful.

/*
 Output:
...
numeric_limits<decimal>::is_specialized is true
...

*/

4 I agree that decimal implies fixed so fixdec is a not very nice name - spare a
thought for those whose native language is not english. I believe the best
namespace is numeric.  I don't think this will deter even COBOL programmers!

5  Overall a much better demo program would be valuable to sell this to average
users.
For example, all the examples in the html doc might be included in trydec.cpp.

The default C locale doesn't produce interesting results, so adding

        cout.imbue(locale("English_United States.1252")); (macroized for other OS?)

would be more 'interesting' if dollar-centric.

6  A test program is needed (promised but ...).  Comments make tests a very
valuable learning tool.

7  The documentation is mainly pitched at a too high a level for users and mixes
detailed rationale with user info.  The 'Boost Standard' format might be more
suitable?

8  The synopsis might be commented better, for example:

    decimal(int, int);  // scale, value

9  I really don't like the name scale at all.  Nor deffrac, decscale and
iosprec. Can't we do better than this, even if longer? These are going to be
very visible in user code, but not at all obvious meaning.

10  You could usefully add an example of explicit limits for the common 32 bit
system

max amount is $21,474,836.47 - OK if you are invoicing from a hardware store,
but for 64 bit
?92,233,720,368,547,758.07 may suit Enron-type accountants better.

Can you give some clues to the probable performance cost of using 64 bits on X86
systems?

Hope this will help you get wide use of this useful code.

Paul