Re: [lmi] Reference member = shared_ptr

[Greg Chicares]

On 2/2/21 3:28 PM, Vadim Zeitlin wrote:
> On Tue, 2 Feb 2021 14:48:49 +0000 Greg Chicares <gchicares@sbcglobal.net> 
> wrote:
> 
> [...snip all the explanations as I have nothing to add...]
> 
> GC> I think everything is reasonable now, as just pushed.
> 
>  I agree, this makes much more sense if the cache is mutable (which is
> something I also had completely forgotten about, sorry). But, just for
> completeness, I'd like to add that it still could be achieved without
> shared pointers: you're right in that using unique_ptr<> wouldn't work if
> its internal pointer changed during the program lifetime, but the simple
> solution is to just not use pointers at all and use objects in the first
> place. Then instead of allocating a new object and assigning it to the
> unique_ptr<> we would just assign the new values to the existing object.
> As long as we keep using the same object from the beginning until the end,
> there is no danger of ending up with a dangling pointer to it.

That's an intriguing idea, and Vaclav had said:

https://lists.nongnu.org/archive/html/lmi/2012-06/msg00007.html
| I ended up using pointers in the end, because DBDictionary is uncopyable
| -- and [...] cannot be easily made copyable.

The present design, with shared_ptr, seems to be the most reasonable
way to obtain the current behavior--if class X HAS-A cached class F,
and instances X0a, X0b, ...X0z are created with instance F0, but
then the file underlying F is changed resulting in instance F1,
then subsequently-created instance X1a, X1b... use F1, but earlier
instances like X0k keep using F0. If we make F a single mutable
object, and replace F0 with F1, then instance X0k would use F1,
which could have astonishing results. For example, maybe F was
changed in a way that would prevent X0k from being created.

Alternatively, we could presumably preserve the current behavior
with some kind of multimap that would preserve all the F0, F1...F99
ever created, but return only the latest F99 for new queries. This
would move the responsibility for extending the lifetime of all FN
into the cache's data structure, instead of just letting shared_ptr
handle that.

Or we could abandon pointers and just return (copies of) objects,
but those copies could be quite expensive.

There are many tradeoffs, and the design we have isn't clearly
worse than any other. It may be unwise to spend time changing it.

>  I admit, I don't know if it's worth making such a change, but I'm just
> very suspicious of widespread shared pointers use. This is something it
> took me a long time to understand, but I'm about as unenthusiastic about
> shared pointers since, maybe, 2015, as I was enthusiastic about them back
> in 2005, because in the intervening years I kept running into deep and
> complicated problems and bugs due to the implicit and very hard to find,
> let alone fix, reuse of the same object from completely different program
> locations via shared pointers to it. Of course, this doesn't happen in
> simple examples and it almost surely isn't going to happen here and, also,
> using shared_ptr<T const> is much better than using shared_ptr<T>. But,
> still, I've earned healthy (or at least I hope that it is) distrust of
> shared pointers after using them heavily in many projects and now always
> try to avoid them unless it's really necessary -- and the surprising thing
> is how rarely it actually is, especially in single-threaded code.

Fascinating. This reminds me of the 1990s debates about how to
design class hierarchies--a forest of independent lightweight
classes, or a single base class that's the parent of everything
(so that you only have to write Print() once, or at least that
was the dream)--which couldn't be resolved by metaphysical
discussion...but then practical experience made the answer clear.

Glancing over lmi's use of shared_ptr, I don't see much that we
really ought to change:

  basic_values.hpp:    std::shared_ptr<product_data       const> product_;

That (and several like it) are shared_ptr because of cache_file_reads.

  basic_values.hpp:    std::shared_ptr<MortalityRates>     MortalityRates_;

That (and several like it) probably arise from the bad old days
of the 1990s, when objects that might exceed 65536 bytes had to
be allocated on the heap. At least we can change these to
unique_ptr; please tell me whether you think new branch
odd/unique_vs_shared should be merged into master.

  account_value.hpp:    std::shared_ptr<Ledger         > ledger_;

Here, class AccountValue does its work and then vanishes (its dtor
executes), leaving behind only the Ledger it produces, which can be
used downstream because the shared_ptr endows it with eternal life.
Isn't that an appropriate use?

  ledger.cpp:// TODO ?? Is it really a good idea to have shared_ptr data 
members?

Probably not. Especially because that comment block speaks of
using 'new' to create a copy, which is a very 1990s solution; and,
again, the original reason for using shared_ptr's here may have
been to work around the 65536-byte limit on an 8086 data segment.
But every time I contemplate class ledger_map_holder, a darkness
descends on me--it's like wishing that you had built your house
out of brick instead of lumber, but it seems like a lot of work
to go back and revise that decision. Still, if you're inclined to
rectify this, I'd be glad to accept patches. Otherwise, when this
comes up every few years, e.g.:

  ledger_pdf.cpp:/// is it time to address the comment about shared_ptr members 
in
    class ledger?

we can just keep kicking the can down the road--it's icky, but
it does have the virtue of actually working.

  multidimgrid_any.hpp:    typedef std::shared_ptr<MultiDimAxisAny> AxisAnyPtr;

Wholesale replacement would be the way to address this and other
concerns with the product-editor code, which constitutes about
one-quarter of lmi's use of shared_ptr.

  rate_table.hpp:    explicit table(std::shared_ptr<table_impl> const& impl)

Perhaps this is a case where you'd still use shared_ptr today?