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

[Vadim Zeitlin]

On Fri, 13 Nov 2020 17:28:40 -0500 (EST) Greg Chicares 
<gchicares@sbcglobal.net> wrote:

GC> chicares pushed a change to branch master.
GC> 
GC>       from  b03a344   Add missing files required for user manual
GC>        new  dec08c1   Enumerate some lingo strings for 'sample' product
GC>        new  180c1d7   Begin to populate 'sample.lingo'
GC>        new  1f36216   Add member function lingo::lookup()
GC>        new  f335cd1   Hold a shared_ptr to lingo in class BasicValues

 Hello,

 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.


GC> branch: master
GC> commit dec08c1c685eabb9995c5a1c1ac10c9536e50b75
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     Enumerate some lingo strings for 'sample' product
GC> ---
GC>  sample.hpp | 30 +++++++++++++++++++++++++++++-
GC>  1 file changed, 29 insertions(+), 1 deletion(-)
GC> 
GC> diff --git a/sample.hpp b/sample.hpp
GC> index 01024ae..ebcfdda 100644
GC> --- a/sample.hpp
GC> +++ b/sample.hpp
GC> @@ -24,6 +24,34 @@
GC>  
GC>  #include "config.hpp"
GC>  
GC> -// Coming soon...
GC> +// For now, this file contains only an enumeration, but someday it may
GC> +// include other information that applies to an entire portfolio.
GC> +
GC> +/// For the fictional Superior Life Insurance Company of Superior, WI.
GC> +
GC> +namespace superior
GC> +{
GC> +/// Enumerate lingo strings.
GC> +///
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.

 The main thing I find annoying with them is that you can't iterate over
them conveniently, even if the values of enum elements are consecutive, but
this doesn't seem to be the issue here.

 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.

GC> branch: master
GC> commit 180c1d7d21ece4a1c971e881bb2e4e34481a6e10
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     Begin to populate 'sample.lingo'
GC> ---
GC>  lingo.cpp | 14 ++++++++++++++
GC>  lingo.hpp |  3 +++
GC>  2 files changed, 17 insertions(+)
GC> 
GC> diff --git a/lingo.cpp b/lingo.cpp
GC> index 00dc4ab..6423837 100644
GC> --- a/lingo.cpp
GC> +++ b/lingo.cpp
GC> @@ -26,11 +26,15 @@
GC>  #include "alert.hpp"
GC>  #include "data_directory.hpp"           // AddDataDir()
GC>  #include "my_proem.hpp"                 // ::write_proem()
GC> +#include "sample.hpp"                   // superior::lingo
GC>  #include "xml_lmi.hpp"
GC> +#include "xml_serialize.hpp"
GC>  
GC>  #include <boost/filesystem/convenience.hpp>
GC>  #include <boost/filesystem/path.hpp>
GC>  
GC> +/// Construct from filename.
GC> +
GC>  lingo::lingo(std::string const& filename)
GC>  {
GC>      xml_lmi::dom_parser parser(filename);
GC> @@ -45,15 +49,25 @@ lingo::lingo(std::string const& filename)
GC>              << LMI_FLUSH
GC>              ;
GC>          }
GC> +    xml_serialize::from_xml(root, map_);
GC>  }
GC>  
GC>  void lingo::write_lingo_files()
GC>  {
GC> +    // superior::lingo enumerators are used for clarity in specifying
GC> +    // this map. They decay to integers in the resulting file, which
GC> +    // can therefore be read without the enumerators being visible.
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).

GC> branch: master
GC> commit f335cd1832c637568cb9f2ee8e1a1254c437775d
GC> Author: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> Commit: Gregory W. Chicares <gchicares@sbcglobal.net>
GC> 
GC>     Hold a shared_ptr to lingo in class BasicValues
GC> ---
GC>  basic_values.hpp | 2 ++
GC>  ihs_basicval.cpp | 7 ++++---
GC>  2 files changed, 6 insertions(+), 3 deletions(-)
GC> 
GC> diff --git a/basic_values.hpp b/basic_values.hpp
GC> index ce3816f..5480e29 100644
GC> --- a/basic_values.hpp
GC> +++ b/basic_values.hpp
GC> @@ -59,6 +59,7 @@ class Irc7702A;
GC>  class Loads;
GC>  class MortalityRates;
GC>  class death_benefits;
GC> +class lingo;
GC>  class modal_outlay;
GC>  class premium_tax;
GC>  class rounding_rules;
GC> @@ -119,6 +120,7 @@ class LMI_SO BasicValues
GC>      yare_input                          yare_input_;
GC>      product_data     const              product_;
GC>      product_database const              database_;
GC> +    std::shared_ptr<lingo>              lingo_;
GC>      std::shared_ptr<FundData>           FundData_;
GC>      std::shared_ptr<rounding_rules>     RoundingRules_;
GC>      std::shared_ptr<stratified_charges> StratifiedCharges_;
GC> diff --git a/ihs_basicval.cpp b/ihs_basicval.cpp
GC> index 21aa86e..341efe2 100644
GC> --- a/ihs_basicval.cpp
GC> +++ b/ihs_basicval.cpp
GC> @@ -41,6 +41,7 @@
GC>  #include "ihs_irc7702a.hpp"
GC>  #include "input.hpp"
GC>  #include "interest_rates.hpp"
GC> +#include "lingo.hpp"
GC>  #include "loads.hpp"
GC>  #include "math_functions.hpp"
GC>  #include "mc_enum_types_aux.hpp"        // mc_str()
GC> @@ -208,6 +209,7 @@ void BasicValues::Init()
GC>              << LMI_FLUSH
GC>              ;
GC>          }
GC> +    lingo_.reset(new lingo(AddDataDir(product().datum("LingoFilename"))));

 This is another rather trivial comment, but it's slightly preferable to
use make_shared(), rather than reset(new ...).

 A less trivial question is whether we need to use shared_ptr at all here.
Looking at the existing members of this type, e.g. FundData_, it seems
unnecessary, as we don't share it with anything. And, of course, the new
member is not shared anywhere neither yet. So unless you do plan to share
it, perhaps we could make do with a unique_ptr<> instead? Those are much
simpler to understand and to follow, unlike shared_ptr<> objects that are
global variables in disguise.

 Regards,
VZ

pgp25yabzCLCi.pgp
Description: PGP signature