Re: [lmi] Help modifying 'rate_table.cpp'

[Greg Chicares]

On 2016-12-15 15:39, Vadim Zeitlin wrote:
> On Thu, 15 Dec 2016 06:02:29 +0000 Greg Chicares <address@hidden> wrote:

[...consistency in number of decimals now tested in validate()...]

> GC> Doing it there, instead of in writer::write_values() (in the "text"
> GC> namespace IIRC) as in my original sketch, also brought the very
> GC> welcome incidental benefit that I didn't have to deal with objects
> GC> of type 'boost::optional const&'. I realize it's awfully late for
> GC> code review, but what was the rationale for 'optional'?
> 
>  I like it. I'd even go as far as saying that I love it. I think it makes
> the code much clear and robust and I try to never use empty strings or
> integers with special sentinel values of 0 or -1 in any new code I write.

It may very well be valuable in other use cases, but the complexity it
adds is not negligible. In an early draft of the above change, I managed
to trigger abort(), presumably by using the wrong assignment operator.
All I wanted to do was change an integer value from six to eight. Doing
that in the most obvious way with an 'int' would not have failed, and in
at least that sense a POD type is more robust. Using the value of an int
simply by writing its name is clearer than prepending an asterisk, and
more robust because there's no asterisk to forget. This thing has a cost.

Consider this simplified example (showing only two of the five invariants):

void actuarial_table::parse_table()
{
    LMI_ASSERT(-1 == table_type_    );
    LMI_ASSERT(-1 == min_age_       );
...(assign real values to all those variables)
    LMI_ASSERT(-1 != table_type_    );
    LMI_ASSERT(-1 != min_age_       );
}

The invariants are immediately clear: at entry, all those member data have
obviously invalid sentinel values; at exit, all have been assigned. These
invariants are limited to the function that enforces them, which guarantees
that they're safe to use elsewhere without asking each one "are you valid?"
before each use. In this particular case, I like sentinels better than any
other technique.

I'll also mention (because it's relevant below) that the 'case' block
where each of those variables is assigned repeats those assertions, e.g.
  assert(-1 == foo);
  foo = 42;
  assert(-1 != foo);
which prevents multiple assignment. (Thus, the assertions at function entry
and exit are redundant and mainly serve a documentary function.)

> GC> I realize
> GC> that some fields such as "Construction method:" may be absent, but
> GC> for text fields like that, wouldn't an empty string do as well? Or
> GC> were any non-text fields missing--e.g., "Maximum select age:" for
> GC> a table that is not select (so that the field wouldn't really have
> GC> any meaning--but I thought it had a required value nonetheless)?
> 
>  There is a technical advantage to using optional<> too: it allows to write
> template write() method that works for both strings and numbers. But the
> main advantage is really that it clearly indicates that a field may or not
> be present in a way that an empty string never could.

I understand this point. However, AFAICT the only fields that are
actually optional are text fields for which there is no meaningful
difference between the value being empty vs. nonexistent. In another
application, a text field might actually have three states:
 - not specified;
 - specified, and empty;
 - specified, and not empty;
but for rate tables the first two states are equivalent. I realize
with regret now that I should have stated that up front.

>  I'm pretty sure that if you tried to rewrite the code without using
> optional<> but keeping its functionality (i.e. the ability to detect
> missing/duplicated fields in the input), you'd end up with a version that
> would be not only longer but also much less clear.

The use of 'optional' stems from a specification defect. I should have
stated up front that missing text fields need not be detected. The
business requirements for this code are fully met if implicitly default-
initialized strings that are never assigned to are treated exactly the
same as strings that are explicitly assigned an empty value.

I understand that if any parameter must be optional, then there's a valid
argument for making all parameters optional so that they can all be handled
the same way. That's why number-of-decimals is physically optional, even
though it is logically not optional. That propagation of optionality
compounds my regret over the specification defect.

> GC> BTW, I looked over my latest change before committing it, and saw
> GC> that I had written
> GC>   *hash_value_ = compute_hash_value();
> GC> with an asterisk, where a few lines lower you had
> GC>   hash_value_ = correct_hash_value;
> GC> with no asterisk.
[...]
>  It's very convenient and std::optional<> in C++17 behaves in the same way,
> so whatever we may think about it, we'll have to live with this.

One must live with the standard, but one needn't like every aspect of it.

> GC> Even though it seems convenient, isn't it astonishing that these
> GC> two statements:
> GC>   *p = 42;
> GC>    p = 42;
> GC> are equivalent?
> 
>  They're not quite equivalent, of course. The first one only works if "p"
> already contains something, while the second one always does. And this is
> why the second is so convenient: you often want to either initialize or
> update a variable, without caring about its old value.

Okay, there's a distinction between them in that context. Still, if I saw
   *p = 42;
    p = 42;
in a C++ puzzle book that says those two statements both result (in some
unspecified context) in changing the value of 'p' to 42, I'd find it
difficult to see how that could be the product of a good design. Maybe
it becomes clear if you've used 'optional', but otherwise it's quite
unexpected at first blush.