Re: [lmi] [lmi-commits] master updated (b03a344 -> f335cd1)

[Greg Chicares]

On 11/13/20 11:55 PM, Vadim Zeitlin wrote:
[...]
>  I've quickly looked over the changes here and have a few remarks below.
> Nothing there is really urgent, so, while I hope that you find these
> comments useful, rather than distracting from your main task, please feel
> free to skip/ignore them and/or let me know if you'd rather avoid, or
> postpone, discussing the details of the code and I won't send them for your
> next commits.

I'm always glad to have your comments. I've almost reached an
important milestone, so I'm going to keep plowing forward for
another day or two, but after that I'll give more thought to
the shared_ptr questions. The others require no incremental
thought, so I'll answer them now:

> GC> commit dec08c1c685eabb9995c5a1c1ac10c9536e50b75
[...]
> GC> +/// This is deliberately defined with enum-key 'enum' rather than
> GC> +/// 'enum class' or 'enum struct'. Because it is defined inside a
> GC> +/// namespace, with an enum-base, it is the same as an 'enum class'
> GC> +/// except that its enumerators decay to int as nature intended.
> 
>  Interesting that you prefer to follow nature rather than something more
> civilized, like type-safety, here. IMHO the lack of decay to int is a very
> appreciable feature of scoped enums.

Perhaps, as a general principle. But in this case automatic conversion
to int is desired. To see why, make 'lingo' an 'enum class' (you may
need to rename it, e.g. to mixed-case "Lingo", to avoid clashes; and
"namespace superior" will no longer serve any purpose, so you'll
probably want to declare it at global scope), and then change the
rest of the code so that everything compiles. I did that experiment
before committing it, and found that I much preferred what I ultimately
committed.

If you really want to do that, though, please wait until tomorrow when
I'll commit some code that actually uses "lingo", so that you get the
full set of error messages. That code, in 'dbdict.cpp', will look
much like this:

         PolicyForm = p.datum(alt_form ? "PolicyFormAlternative" : 
"PolicyForm");
+if("sample" == ProductName)
+    {
+        auto policy_form = b->database().query<int>(DB_PolicyForm);
+        std::string PolicyForm_NEW = b->lingo_->lookup(policy_form);
+// Either way, the result is the same:
+LMI_ASSERT(PolicyForm_NEW == PolicyForm);
+    }
         PolicyMktgName             = p.datum("PolicyMktgName"                 
);

>  Personally I'd both use scoped enums and serialize them as strings, too,
> as using raw integers has always ended up being problematic IME, but maybe
> I'm missing some deeper reason to prefer using integers here, of course.

There are a couple of particular reasons. One is that '.database' files
contain only type 'double'--not because floating point is always wanted,
but because 'double' can hold any numeric value that would make sense
for product parameters. Many parameters are already boolean or integral,
and existing code depends on their being convertible to double; that's a
convenience that I want to preserve.

The other reason is that this enum is not intended as an integer type
with an enforced range restriction; instead, it's intended to be an
integer with a name that can be freely used in proprietary code, but in
public code is ineffable (so serializing the names as strings is
politically unacceptable). The rationale is given here:

  
https://git.savannah.nongnu.org/cgit/lmi.git/commit/?h=odd/string_db3&id=9250a58b76150804c4aeaa4f5295eafd155c884e
+// New way: establish a compendium of company-specific strings, and
+// store indexes into that compendium in database_entity objects.
+// The only question is what form that compendium should take.
+// Answer: one XML '.lingo' file to be shared by a company's entire
+// portfolio of products. It is most convenient to associate each
+// string with an enumerator, to provide a descriptive name, but some
+// such names might be particular to the company, e.g.:
+//
+// static std::string const S_DefnCSV = // default for most products
+//   "Cash surrender value is account value less any surrender charge.";
+//
+// static std::string const S_DefnCSV_turbo =
+//   "Cash surrender value is account value less any surrender charge,"
+//   " plus the Turbo Asset Boost provided by a special endorsement.";
+//
+// ...where each product-file entity already has a name, and its
+// string values are named with an "S_" prefix, and if necessary a
+// suffix indicating specialization--so a specialized footnote for a
+// particular product might naturally be named with a "_turbo" suffix,
+// for which a different company would have no use. Today's product
+// files might have
+//  item("DefnAV")                     = S_DefnAV;
+// in the generic case, and
+//  item("DefnAV")                     = S_DefnCSV_turbo;
+// for "turbo" products. In (proprietary) code that creates that
+// company's product files, it's convenient to provide a "turbo"
+// enumerator:
+//  enum e_lingo
+//      {e_defn_av
+//      ,e_defn_av_turbo
+//      };
+//  static std::unordered_map<e_lingo,std::string> m
+//      {{e_defn_av, S_DefnAV}
+//      ,{e_defn_av_turbo, S_DefnCSV_turbo}
+//      };
+// and use it to replace '.policy'-generation code like this:
+//  item("DefnAV") = S_DefnCSV_turbo;
+// with '.database'-generation code like this:
+//  Add({DB_L_DefnAV, e_defn_av_turbo});
+//
+// The proprietary "turbo" feature doesn't leak into lmi's public
+// code, because a proprietary 'e_lingo' is used only in proprietary
+// code that writes an XML file with integer rather than enumerative
+// keys, which are read into a map<int,std::string> in public code.
+//
+// What about strong typing? That's present on the write side, where
+// it can be used to ensure that no invalid "lingo" file is written.
+// Therefore, it's not very important on the read side, where it could
+// only verify the validity of a file already known to be valid. It's
+// not really needed on either side: actuarial-table numbers are naked
+// integers, yet they have worked just fine for decades.

One of the important remaining tasks is to say that in fewer words
as inline documentation.

> GC> commit 180c1d7d21ece4a1c971e881bb2e4e34481a6e10
[...]
> GC> +    static std::unordered_map<superior::lingo,std::string> 
> enumerative_map
> GC> +        {{superior::policy_form      , "UL32768-NY"}
> GC> +        ,{superior::policy_form_KS_KY, "UL32768-X"}
> GC> +        };
> 
>  This is a much more trivial comment, but it looks like this map ought to
> be "const" (I don't think it can be made constexpr in C++17).

Thanks. Committed but not yet pushed.

Does your phrase "in C++17" suggest that it can be made constexpr
in C++20? We're still using gcc-8.1 in production, but we intend
to switch to gcc-10 by about the end of the year.