[lmi] Reference member = shared_ptr [Was: master updated (b03a344 -> f335cd1)]

[Greg Chicares]

On 11/13/20 11:55 PM, Vadim Zeitlin wrote:
> On Fri, 13 Nov 2020 17:28:40 -0500 (EST) Greg Chicares 
> <gchicares@sbcglobal.net> wrote:
[...]
> GC> branch: master
> GC> commit f335cd1832c637568cb9f2ee8e1a1254c437775d
[...]
> GC>     Hold a shared_ptr to lingo in class BasicValues
[...]
> 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> @@ -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> @@ -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.

These really are intended to be global variables--initialized on
demand, and persisting until program termination--at least since
commit 6620be4ea55 of 2021-01-28, which of course didn't exist
when you wrote that comment; but now is a good time to step back
and reconsider the way shared_ptr is used in these cases.

Caching is of demonstrably great value:
  git show 6620be4ea5 -- Speed_gcc_*
and 'cache_file_reads.hpp' seems like a good way to achieve it
(i.e., as a map of shared_ptr<file_data> keyed by file_path).
And shared pointers to classes like FundData are appropriate even
if they were used only in class BasicValues, because they're
shared across the many BasicValues instances that may be created
in one lmi session.

But how should this cache be used?

Strategically, I think the present way is fine: everything's
a shared_ptr. These product files are expensive to parse, but
don't use much memory; there aren't very many of them (a few
hundred at most); and lmi sessions may last hours, but not
weeks--so we never have any reason to discard anything from
the cache, except at program termination; thus, I see no
reason to use weak_ptr here.

Tactically, however, I'm not so sure about all the variations:

 - class BasicValues holds a
    std::shared_ptr<product_data>       product_;

which seems fine (what could be more natural?);

 - product_database::initialize() has a local const reference:
    product_data const& p(*product_data::read_via_cache(f));

which seems fine because it's local (beware of the rather
ill-chosen class names: product_database != product_data);

 - class product_verifier has a const-reference data member:
    product_data     const& p_;

which seems...well, I dunno. I hesitated to do that, but
then I reasoned that this refers to cached data that's never
discarded, so it seems okay. But if we decide that's really
okay, then should we hold a 'product_data const&' in the
first example as well (class BasicValues), for uniformity?

Or is initializing a reference member from a shared_ptr
anathema?