Re: [lmi] New rate-table implementation

[Greg Chicares]

On 2016-05-19 13:33, Vadim Zeitlin wrote:
> On Thu, 19 May 2016 00:40:24 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2016-03-19 19:43, Greg Chicares wrote:
> GC> [...]
> GC> > Therefore, 'actuarial_table.?pp' should be replaced in its entirety 
> after
> GC> > thorough testing.
> GC> 
> GC> Replacement candidate:
> GC>   https://github.com/vadz/lmi/pull/23/
> GC> committed 20160519T0031Z, revision 6603.
> 
>  Thank you for committing this, but I'm not sure if we're attaching the
> same meaning to the word "replacement": I was speaking about replacing the
> existing code in actuarial_table.cpp with the code in what is now known as
> rate_table.cpp.

It's a preliminary candidate for possible eventual replacement of existing code.

> So this replacement hasn't been even started yet, let alone
> achieved candidate status. The question is: should I start it now?

I don't think so. I'd like to read it first.

> GC> I made some superficial changes before committing:
> GC>  - smite files with BDFL rod of renaming +/-7 (sign depends on your POV)
>                                                                         ^^^
>  Didn't you mean "alignment"?

Indeed. Positive sign if you're lawful chaotic like me. (All chaotic good
characters see themselves as lawful--i.e., serving a higher law; and, to
that end, are allowed to correct the system of alignments.)

>  Anyhow, to continue my preaching to the converted, this renaming didn't
> bother me at all because of Git content-addressable nature: it knows that
> mort-tables:soa_database.cpp master:rate_table.cpp are the same files and
> so can show me the (small) difference the two, for example. With svn it
> would have been much more fun...

We must migrate to git. I'm slaying RCS Id's now.

> GC>  - use regexen instead of substrings in tests of thrown exceptions
> GC>      (I strove to escape periods ('.'), but didn't exercise any real
> GC>      cleverness in turning these into regular expressions)
> 
>  BTW, you may want to use C++11 raw string for regexes, not needing to
> escape backslashes in regexes is really helpful.

I resisted the temptation to delay the patch in order to make that
desirable change, especially because I might have introduced a defect
through misunderstanding. It would be nice to use raw strings elsewhere,
too, e.g.:
    oss << "Copyright \\(C\\)[^\\n]*" << year;
    static boost::regex const forbidden("[\\x00-\\x08\\x0e-\\x1f\\x7f-\\x9f]");

> GC>  - reformat comments explaining header inclusion (thanks for following
> GC>      a style consistent with existing code...but that was Beman Dawes's
> GC>      code, which I recently reformatted, so I did the same here)
> 
>  Sorry about this, I'm still not sure what the rule here is. But all in all
> there are very few changes, so I'm quite happy about it ("it" being either
> the success of my attempts to conform to lmi coding style or you deciding
> it was not worth to deal with my failure to do it...).

I've been moving gradually to a rule that I'll state by visual example:

00000000011111111112222222222333333333344
12345678901234567890123456789012345678901
#include "wherever/whatever"            // whatever::frobnicate()
#include <whatever>                     // std::frobnicate()

Forty characters should be enough for an include-directive. Where it's not:
#include <boost/filesystem/convenience.hpp> // fs::extension()
we'll just have to wait for C++17.

> GC> I clumsily revised one use of the stdout redirector because it seemed
> GC> to gobble a useful error message.
> 
>  Sorry, I don't see it, which one?

This one, which used the redirector in your original:

void test_from_text()
{
    // Using unknown header in a place where it can't be parsed as a
    // continuation of the previous line should fail.
    {
    std::cout << "Expect 'Possibly unknown field...':" << std::endl;
    BOOST_TEST_THROW
        (table::read_from_text("Bloordyblop: yes\n" + simple_table_text)
        ,std::runtime_error
        ,lmi_test::what_regex("expected a field name")
        );
    }

I conjecture that you'll see the same problem I saw if you change it thus:
-        ,lmi_test::what_regex("expected a field name")
+        ,"THIS DOES NOT MATCH"
but with all my RCS Id changes pending, I'm indisposed to test that at
the moment.