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

From: Fernando Cacciola (fernando_cacciola_at_[hidden])
Date: 2003-07-14 09:29:27

Hi, this is my review of the Fixed-Point Decimal Library.

I cannot vote as it is now.
I will vote subject to the resolution of the 'scale' issue I explain below:

Problems with 'scale':
======================

To understand it, I wrote the following function which just creates two
decimals given a source integer and two scales:

void Show( int s, int scn, int scm)
{
  decimal n(scn, s);
  decimal m(scm, s);

  cout << "s=" << s << endl
       << decscale
       << "[" << scn << ",s]=" << n << endl
       << "[" << scm << ",s]=" << m << endl
       << endl ;
}

When called with:

  Show(numeric_limits<int>::max(),9,10);
  Show(9,17,18);
  Show(10,17,18);
  Show(0,17,18);

the output (in a system were int_type is __int64) is:

s=2147483647
[9,s]=2147483647.000000000
[10,s]=302809239.6290448384

s=9
[17,s]=9.00000000000000000
[18,s]=9.000000000000000000

s=10
[17,s]=10.00000000000000000
[18,s]=-8.446744073709551616

s=0
[17,s]=0.00000000000000000
[18,s]=0.000000000000000000

Furhetmore, the following:

  decimal eps =  numeric_limits<decimal>::epsilon() ;
  decimal a = eps + 9 ;
  decimal b = eps + 10 ;
  cout << decscale
       << numeric_limits<decimal>::digits10
       << eps
       << a << endl
       << b << endl ;

outputs:

18
0.000000000000000001
9.000000000000000001
-8.446744073709551615

These results seems to clearly show that 'scale' is actually the maximum
number of decimal digits allowed IIF only one digit is used for the whole
part.
If the whole part uses more than one digit, say N, the decimal part won't be
able to represent 'scale' digits, it will only represent 'scale+1-N' digits,
as shown by the following:

max with scale=0 : 9223372036854775807
max with scale=1 : 922337203685477580.7
max with scale=2 : 92233720368547758.07
max with scale=3 : 9223372036854775.807
max with scale=4 : 922337203685477.5807
max with scale=5 : 92233720368547.75807
max with scale=6 : 9223372036854.775807
max with scale=7 : 922337203685.4775807
max with scale=8 : 92233720368.54775807
max with scale=9 : 9223372036.854775807
max with scale=10 : 922337203.6854775807
max with scale=11 : 92233720.36854775807
max with scale=12 : 9223372.036854775807
max with scale=13 : 922337.2036854775807
max with scale=14 : 92233.72036854775807
max with scale=15 : 9223.372036854775807
max with scale=16 : 922.3372036854775807
max with scale=17 : 92.23372036854775807
max with scale=18 : 9.223372036854775807

Notice that the total number of represtanble digits is scale+1.
Therefore, I really dislike the term 'scale' and its explanation.
IMO, it should be called: 'digits', and it should be defaulted to 1 +
[int_type].digits10
(i.e 19),
Additionally, numeric_limits<decimal>::digits and digits10 should be
max_scale+1,
not max_scale.

Here is a list of other minor issues:
=====================================

I don't think that is_bounded and is_modulo should be inherited from
int_type.

is_bounded is intended to be false only for those types which can represent
numbers in any range. Even if int_type were unbounded (is_bounded=false),
decimal itself will always be bounded, so it should fix is_bounded to true.

Similarly, is_modulo tells whether the numeric type uses modulo arithmetic,
as do the unsigned integral types, yet decimal does not do that even if
int_type where unsigned, so this should be fixed to 'false'.

***************************

Currently, the library cannot be put in a precompiled header using BCC
because of the 'max_val,min_val' constants. This can be solved by making
those constants inlined functions.
I can send you a patch if you like.

****************

The documentation is clearly written with some 'old' version in mind. I
think all references and comparisons with that older version should be
removed from the docs, or at least, put aside on a 'history' section.

*************

IMO the documentation should begin with a brief (but complete) overview of
what is a 'fixed decimal' number, including the concept of a 'scale' (or
'digits' as I much prefer), and of rounding modes.

******************

I understand why it is not supported to construct a decimal from an
int_type, but I think that construction from 'unsigned int' should be
supported since, AFAICT, the promotion rules won't give ambiguity if that
additional ctor is added.

************

The docs say:

"The conversion will be exact if the string contains no more than scale
digits to the right of the decimal point; otherwise the value will be
rounded as specified"

This is incorrect, I think. If the string contains more than (scale+1)
digits, whether to the left or right of the decimal point (or both),
rounding will ocurr.

**********

Best,

Fernando Cacciola