[boost] Re: Thoughts on fixdec

From: Daryle Walker (dwalker07_at_[hidden])
Date: 2003-07-17 17:46:52

So far, I can run the code though my copy of Metrowerks CodeWarrior Pro 
(v.8), but only in debug mode!  It doesn't work on Project Builder 
(which uses GCC 3.1).  The latter chokes on missing definitions of 
"int_type".  I don't know if the problem is my setup or if your code 
structure is confusing the parser.

I forgot two more methods.  You have operators "++" and "--" to change 
the number by 1 (absolute), but no methods to it by 1 ULP (which would 
apply "++" or "--" to the significand).

class decimal
{
public:
     //...
     decimal &  goto_next();
     decimal &  goto_previous();
     //...
};

decimal  next( decimal const & );
decimal  previous( decimal const & );

Also, if you add an accessor for the significand, that and your 
existing scale-accessor together would eliminate the need for those 
extra semi-secret methods (or any need to make your functions 
friend-ly).

Now onto your comments:

On Wednesday, July 16, 2003, at 7:55 AM, Bill Seymour wrote:

> Daryle Walker wrote:
[SNIP]
>> 5.	The conversion from "int" needs to check for overflow.
>> The conversion from "double" needs to check also for underflow.
>> The conversion from strings needs to check also for bad formats.
>>
>
> But that would require some users to pay for something they don't
> need.  Users who need that level of correctness can write their
> own checks; and others (probably most of the library's target
> audience) get better efficiency.

Speed without some sort of safety isn't too useful.

You could add a derived class or alternate constructors that do the 
checks.  Or, at least, change the documentation to explicitly state 
that out-of-bounds or otherwise inappropriate constructor values give 
undefined behavior.

>>
>> 6.	There should only be conversions from strings, and _no_
>> mixed operations with strings.
>>
>
> In the absence of decimal literals, I think it's easier for the
> user to be able to use strings as pseudo-literals in all contexts;
> and I don't see how the mixed operations do any harm.  In particular,
> I'm willing to give the user credit for knowing that
>
> 	my_decimal += "The quick brown fox...";
>
> doesn't make any sense.

But just because people don't usually write "x = x" doesn't mean 
certain classes shouldn't write assignment operators with 
self-assignment-safety in mind.  As another poster said, you're opening 
yourself up to not-so-obvious conversions.  It's the 
speed-versus-safety argument.  Would you really lose any speed over 
forcing the user to do:

     my_decimal += decimal( "-5.4" );

Your special operator has to do something like this internally anyway.

>>
>> 7.	Does the scale need to be first in all constructors?  If it
>> can be placed second, then converting constructors can be made
>> and most of the mixed operator functions can go away.
>>
>> [ctor examples]
>>
>
> I'd want to see use cases for all those conversions before I agreed
> to add them.  Remember, the library's target audience is folk who
> are writing financial software.  It's not intended as a general-
> purpose number-crunching library.

The constructor set I gave is basically like the one you had.

A.	I used the maximum signed-integral, unsigned-integral, and floating 
built-in types so constructors for all the other built-in numeric types 
don't need to be specified since they can use the built-in promotions.

B.	If you're going to have constructor for C-style strings, you might 
as well add one for C-style wide strings.  (Those constructors are 
explicit due what I said in [6].)

C.	I switched the order of the value and scale arguments, so defaults 
can be used for the scale argument, enabling the constructors to be 
used as conversions.  Since you would now have conversions, you can 
dump that workaround of having to write explicitly every combination of 
operator parameters.  Less code means less opportunity for mistakes and 
less time needed for everyone to wade through and check the code.  (If 
you're mindlessly repeating code, you're probably doing something 
wrong.)

D.	You won't have a constructor that takes only the scale.  Just hold 
off construction of a value until you have both the value and the 
scale.  You can't have your "cute" assignment semantic of retaining the 
scale, but I want that gone anyway.

E.	Is there some need to put the scale first?  Putting it second allows 
conversion syntax, which the non-hardcore programmers you're aiming for 
can adjust to easier.

>>
>> 8.	The "round_down" and "round_up" functions don't seem clear enough.
>>
>
> They're intended to be clear to accountants who deal only with
> non-negative debits and non-negative credits; and whether a value
> is a debit or a credit is orthogonal to the mathematical notion
> of sign.
>
> Remember the instructions on your tax return form that tell you,
> if line x is less than line y, subtract line x from line y and
> write the answer over here; otherwise subtract line y from line x
> and write the answer over there?  Sure, that's a howler for
> mathematicians; but it makes perfect sense to accountants. 8-)

You could keep the six combinations I gave before, but map two more 
functions "round_up" and "round_down" to "round_fleeing_zero" and 
"round_towards_zero", respectively.

[SNIP]
>> 10.	The class is no longer a template, but some of the wording
>> in the code and docs act like it's still a template.  (When you're
>> rewording, there's some HTML errors to fix.)
>>
>
> Could you point them out?

On my browser (Safari 1.0 [for Mac OS X]), everything on and after the 
constructors in the "Brief overview" is in typewriter font.  You 
probably forgot to close a tag somewhere.

>>
>> 11.  ...
>>
>> ...
>>
>> class MyNum
>> {
>> public:
>>      //...
>>      bool is_negative() const;
>>      //...
>> };
>>
>> ... would save time over doing a (possible) conversion from zero
>> and a comparison.
>>
>
> But (my_decimal < 0) doesn't do a conversion; and it lets the user
> use decimals the way they would use other arithmetic types.

It would be a conversion if [7] is applied.  I was stating an opinion 
that "is_negative" should be an API that all (class-based) real-number 
numeric types support from now on.  (We could later make a 
function-class and function around it, specializing for the built-in 
types.)

>>
>> 12.	[use pword to save rounding mode I/O state]
>>
>
> Yes, much better.  I'll play with that.

If you have to use dynamic allocation (if a code pointer can't fit in a 
data pointer), make sure to read up on the format copying callback 
mechanism.

>>
>> 13.	You have strange (regular) assignment semantics.  You try to
>> change the receiver's logical value to the source's logical value,
>> but retain the receiver's original scale (internally shifting the
>> new significand and doing any rounding if needed).  ... you will
>> get strange effects if you try to change containers of "decimal"
>> through assignment.
>>
>
> I thought it would be less surprising if a decimal object's scale
> were immutable.  This matches prior art (the BCD type on IBM
> mainframes, for example) in which the scale is part of the type.

But the scale isn't part of the type (otherwise, it would be a template 
parameter), so don't lie and pessitimize your class's implementation.

What you have is more of a surprise because (as another poster said) it 
breaks the C++ usual-object model.  This means that this class is 
INCOMPATIBLE WITH THE STANDARD LIBRARY!!!!!!!  (std::auto_ptr<> also 
has this problem, but it was deliberate and its documentation mentions 
this fact.)

Do you want to force your non-hardcore programmer users to re-implement 
the standard library?

Or worse, they won't realize the problem until they get funky results.  
Even then, they may have to get a C++ guru to figure out the cause, 
wasting everyone's time all-around.

> You're right about container<decimal>; but remember that this class
> isn't intended for number-crunching; so I don't really care about
> assigning matrices, etc.

Your giving concern for speed and efficiency over safety, so you have a 
number-crunching class!  (It just isn't a hardcore one like BLAS.)

And by "container," I wasn't talking about mathematical arrays, I was 
talking about the C++ container classes.  Would your users be happy to 
find out that they can use only single objects of decimal, and can't 
group & manipulate them in containers?

If you really want to have your current assignment semantics, spell 
them out explicitly in another method.

class decimal
{
public:
     //...
     void assign_keeping_old_scale_internally( decimal const & );

     void  internally_rescale( int scale_delta, rounding_mode 
potential_rounder = default_rounding() ) const;
     //...
};

The latter method would need mutable data members.  Maybe the best 
solution would be a custom copying constructor:

class decimal
{
public:
     //...
     decimal( decimal const &d, int internal_scale, rounding_mode m = 
default_rounding() );
     //...
};

The automatic copy constructor would act like this with 
"internal_scale" set to "d.scale()".  An equivalent component 
constructor would be:

class decimal
{
public:
     //...
     decimal( intmax_t mantissa, int exponent, int representation_scale, 
rounding_mode m = default_rounding() );
     //...
};

The value would be as if the scale was "representation_scale" and the 
significand was "mantissa * 10**exponent".

[SNIP]
>> 15.	It took me a while to figure the following out, so maybe you
>> should put it in simpler language.  Objects of this type retain a
>> copy of the given value, rounded (via the given mode) and kept to
>> a precision of the given scale number of (base-ten) fractional digits.
>> The value is stored as that scale and the (integral) number of
>> 10**(-scale) units needed to add up to the value.
>>
>
> I'm not sure what you mean.

I read the documentation several times, but I still didn't get what 
kind of numbers you were representing.  I think it is:

        logical value = significand / 10**scale

Right?

>>
>> I guess that the scale needs to be non-negative?
>>
>
> Yes, and that's pointed out in the documentation, but maybe not
> loudly enough.
>
>>
>> 16. ... the "[]" is bound to the object on the left (by C++'s parsing 
>> rules), and not to the operator.  You just adjust the spacing and 
>> wish that the brackets were associated with the operator on the >> right.
>>
>
> Yes, and that's pointed out in the documentation with a box around it; 
> and there's a rather long bit (maybe even too long) that explains how 
> the whole business came into being.

I don't think there's enough documentation on how that bracketing mode 
affects operations.

>>
>> 17.	Do you really need to support VC++5 ...?
>>
>
> Yes, I do, because that's what I'm stuck with at work (for what are
> euphemistically called "non-technical reasons").  I understand
> completely that such support is not required for Boost; but absence
> of a requirement isn't a requirement of absence; and I don't see
> that the additional support does any harm.  (Users have to hack
> their visualc.hpp to let that version pass anyway.)

It could be a problem if the workaround is the same size, or larger, 
than the main code.  A lot of reading needs to be done to distinguish 
the two sections.

There's a lot of extra code to give the VC++ 5 version the same 
capabilities of the standard version.  Couldn't it just be no-frills?

Daryle