Re: [lmi] Currency class

[Vadim Zeitlin]

On Sun, 20 Sep 2020 13:54:15 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 2020-09-19 21:57, Vadim Zeitlin wrote:
[...]
GC> > - I thought currency(0) was fine if a bit long, but currency{} is IMO much
GC> >   less readable. I think it would be worth defining "_c" literal suffix, 
as
GC> >   mentioned in the README and using "0_c" instead.
GC> 
GC> See
GC>   commit d7fcd483b Write zero cents more tersely

 I've actually thought about adding a constant for currency{0}, but decided
that it wouldn't really look as good as 0_c. OTOH adding user-defined
literals support if the only literal that is ever going to be used with
this suffix is 0 looks strange too.

 But OT3H, if we really only care about 0, could we actually use the fact
that 0 is the only integer constant which is implicitly convertible to
(any) pointer to our advantage? Consider such minimal currency class:

---------------------------------- >8 --------------------------------------
class currency
{
public:
    constexpr          currency(struct zero_tag*)                 {}
    constexpr explicit currency(int cents)        : cents_{cents} {}

    auto operator<=>(const currency& c) const = default;

private:
    int cents_ = 0;
};
---------------------------------- >8 --------------------------------------

AFAICS this would work exactly as we'd like it to, i.e. allow using just
literal 0 in all comparison operations involving currency, but avoid
implicit conversions in all the other cases.

 It does look a bit ugly and I'd definitely add a comment explaining what's
going on here if I decided to use it, but other than that, does this
approach have any drawbacks?


GC> The readability of "currency{}" seems to be a matter of opinion.

 Yes...

GC> Having made changes like this:
GC> -    return round_minutiae().c(z * std::max(currency(), 
TotalAccountValue()));
GC> +    return round_minutiae().c(z * std::max({}, TotalAccountValue()));
GC> I now find even "std::max({}, some_variable)" perfectly clear; and it
GC> has the great advantage that I can change the type of 'some_variable'
GC> without having to change "{}". In that way, it's like 'auto'; adherents
GC> of the "auto everywhere" school of thought logically must like this too.

 It's the reverse side of the medal: omitting the type is obviously good if
you don't care about what this type is, but equally obviously bad if you do
care about it. I don't know in which situation are we in this case. Right
now it seems confusing because I do want to understand what's going on and
whether we're using doubles or currencies, but I guess that most of the
time this wouldn't really be very important, so maybe hiding the types is
indeed a good thing.

GC> > - I'd definitely get rid of the "m" accessor.
GC> 
GC> It's a tradeoff. I wanted operator==() and operator<() to be
GC> non-member non-friends, and a public 'm() const' seemed an
GC> acceptable price to pay.
GC> 
GC> It's generally unreasonable to ask an object to show the raw
GC> value of a private member. But it's not unreasonable to ask
GC> a currency object how many cents it represents. Would your
GC> objection be well answered by a s/\<m(/cents/g renaming?

 Yes, definitely. A cents() method looks perfectly reasonable and much more
noticeable/greppable.

GC> >   Going in the other direction, some people would recommend writing this 
as
GC> > 
GC> >     auto foo = currency(expr);
GC> > 
GC> >   which could work too, if we/you want to favour this "auto-almost-always"
GC> >   style
GC> 
GC> Changing the type of so many variables from 'double' to 'currency'
GC> made me think of that again. The benefit is that in
GC>   :%s/double/currency/gc
GC> the '/c' part (ensuring each change is correct) is a lot of work
GC> much of which would be unnecessary with 'auto' nearly everywhere.
GC> The disadvantage is that it's harder to discern a variable's type.
GC> As a corollary, if we change
GC> - double foo() {...}
GC> + currency foo() {...}
GC> then an expression like
GC>   auto bar = foo() * 13.0 + quux();
GC> might break in some way that's hard to see.

 I think the goal should be to provide such an API in the currency class
that such expressions couldn't break. But I'm not sure if it's doable, as
it would definitely involve removing the ctor from double, in order to
force specifying the rounding rule.

GC> But if we break things only in a branch, then reimplement cleanly
GC> in 'master', we can cleave the 'master' commit into
GC>  - phase one:
GC>      s/double/auto/  where we can
GC>  - phase two:
GC>      s/double/currency/  only where we need to
GC> Phase one is pretty easy to do reliably,

 In theory, this phase could break something, as the type might change to
something other than double, but it could be automated in a perfectly safe
way, AFAICS:

1. Replace all "double \(\w\+\)" with "auto /* UNIQUE_MARKER:\1 */ \1".

2. Run

        %s/UNIQUE_MARKER:\(\w\+\)\(.*\)$/UNIQUE_MARKER:\1\2; 
static_assert(std::is_same_v<decltype(\1), double>);/

3. Compile the code to ensure that the types haven't changed.

4. Remove the scaffolding:

        %s@/\* UNIQUE_MARKER:\(\w\+\) \*/\(.\+;\) static_assert.*$@\1@

(BTW, I've checked and "UNIQUE_MARKER" indeed doesn't appear in lmi sources
right now).

GC> > - Almost all uses of doubleize() are in material_difference() argument
GC> >   list. My first idea was to provide materially_equal() overload taking
GC> >   currency directly. My second, and I think, better, idea was to remove
GC> >   calls to it entirely as 2 currency objects can't be materially equal if
GC> >   they're different, i.e. it looks like we could just use the normal
GC> >   difference for currencies instead. But there is a somewhat cryptic
GC> >   "um...yes, it is" comment in ihs_acctval.cpp implying that we can't do
GC> >   this, so I must be missing something here -- but what?
GC> 
GC> IIRC, I replaced that occurrence with an "==" comparison, and then
GC> found that I had broken something. But maybe that was back when
GC> I was experimenting with
GC>   using currency = double;
GC> which of course can't work now with all the d() calls.
GC> 
GC> BTW, those d() calls are like painting vermin with a fluorescent dye.
GC> The goal isn't just to make them visible, but to eradicate them.
GC> I would hope that the eventual 'master' will have a lot fewer d()'s.
GC> 
GC> Anyway, some floating-point differences are immaterial:
GC>   assert(1.0 == 3.0 * 1.0/3.0);
GC> but no integral currency difference is material. However, I'm
GC> also thinking about defining 'currency' as a floating-valued
GC> class that's scaled by 100, so that it can hold e.g. $1.23
GC> as the exact integral value 123¢, but allowing it to assume
GC> non-integral values; in that case, material equality still
GC> matters.

 Hmm, wouldn't this just hide the problem without really solving it?


 Thanks for taking care of all my other comments!
VZ

pgpJIqedVZRn8.pgp
Description: PGP signature