Re: [lmi] Non-null smart pointer

[Greg Chicares]

On 2018-10-15 22:27, Vadim Zeitlin wrote:
> On Mon, 15 Oct 2018 22:15:52 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 13/10/2018 22.54, Vadim Zeitlin wrote:
> GC> > On Sat, 13 Oct 2018 22:08:48 +0000 Greg Chicares <address@hidden> wrote:
> GC> > 
> GC> > GC> Does lmi need exactly one kind of smart pointer, or more than one?
> GC> > 
> GC> >  Right now I can't point to any precise place where we need either
> GC> > shared_ptr<> or scoped_ptr<> or even unique_ptr<> which can be null. But
> GC> > this doesn't mean there any no such places.
> GC> 
> GC> We can't demonstrate that there are no such places now, or that none will
> GC> be introduced in the future. That's a problem.
> 
>  Sorry, but why is this a problem? It would only become a problem if/when
> we decided to get rid of shared_ptr<> and unique_ptr<> completely
> (replacing the later with not_null_unique_ptr<>), but how is it a problem
> right now?

We're actually talking about two different things.

You're saying that, of all the places where we use smart pointers, you
don't immediately notice any where the value could _appropriately_ be
null. That's not a problem: any such situation will make itself manifest
pretty quickly.

I'm saying that we don't know whether any of our smart pointers could
potentially be null where null is _inappropriate_. That, of course, is
the problem that not_null_unique_ptr<> would solve (except where we
really need not_null_shared_ptr<>, but I don't think we will need that).

> GC> > GC> Do all our smart pointers need to be returnable from a function,
> GC> > GC> or only some of them? Do any actually need reference counting?
> GC> > 
> GC> >  I can try to find the definitive answers to these questions, but it's
> GC> > going to take some time and, to be honest, I'm not sure why are they
> GC> > important. What matters, IMO, is that we want to have a never null 
> owning
> GC> > smart pointer which can be returned from a function and that right now 
> we
> GC> > don't have such a thing.
> GC> 
> GC> If we were to design such a thing now, before figuring out what we
> GC> actually need, we might later find that our real needs haven't been met.
> 
>  Assume that not_null_unique_ptr<> doesn't meet all our needs, i.e. that we
> do need shared_ptr<> and/or that we need unique_ptr<> which may ne null.
> How would this change that the fact that we do need not_null_unique_ptr<>?

I've surveyed lmi's usage of smart pointers in general, and I think we
need far fewer of them, and dramatically fewer shared_ptrs in particular.
But now I'm wondering whether we often want any kind of smart pointer at
all. For example, in 'group_value.cpp', we have:

    std::vector<std::shared_ptr<AccountValue>> cell_values;
      for(...)
        std::shared_ptr<AccountValue> av(new AccountValue(ip));
        cell_values.push_back(av);

which we could rewrite thus:

    std::vector<std::unique_ptr<AccountValue>> cell_values;
      for(...)
        std::unique_ptr<AccountValue> av(new AccountValue(ip));
        cell_values.push_back(std::move(av));

That seems like a positive change, but is it the best we can do? I'm
just now learning that container<T> no longer requires that T be
Copyable (which class AccountValue is not). I haven't tried this yet:

    std::vector<AccountValue> cell_values;
      for(...)
        cell_values.ADD_ELEMENT_SOMEHOW(AccountValue(ip));

but if that works, is there any need for pointers here at all?

Might we indeed find that we almost never need smart pointers?
Do we need them only for "virtual ctors"? and how often do we
need to create an object without knowing its concrete type?

>  I think we should start by adding and using not_null_unique_ptr<> in the
> places in which it is better suited than unique_ptr<> (and again, we
> already know that this is a non-empty set). We know that it won't be able
> to replace all instances where different other smart pointers are used, but
> so what?

Even better than a smart pointer that's constrained to be non-null
is an object that by its nature cannot possibly be null.

> GC> That's a well-defined bucket, independent of lmi's present use of standard
> GC> smart pointers. Is gui_ptr<> small enough to show its implementation here?
> 
>  It's trivial. I.e. basically it's just
> 
>       template <typename T> using gui_ptr = T*;
> 
> As I said, I'm not really sure if it's worth using.

Oh, I thought perhaps you had written it with a non-null constraint.

> GC> >  So, what's the current state of discussion? Do we decide that we
> GC> > 
> GC> > 1. Want to use not_null<T*>? And, if so, do we want to take some 
> existing
> GC> >    implementation or write our own version (which should be simple 
> enough
> GC> >    but OTOH why reinvent even a simple wheel?)?
> GC> > 
> GC> > 2. Want to use not_null_unique_ptr<T*> which would be used for 
> returning 
> GC> >    new objects from functions?
> GC> > 
> GC> > 3. Or need more time to review all the existing (smart) pointer usage in
> GC> >    lmi before making the decision?
> GC> 
> GC> The third. We have several problems:
> GC>  - Dereferencing pointers that might be null.
> GC>  - Using shared_ptr where unique_ptr would be better.
> GC>  - Using smart pointers where concrete references would be better.
> 
>  I'll try to look at all the various cases where shared_ptr<> can be
> replaced with unique_ptr<>,

I've already done some work there for
  group_quote_pdf_gen*.?pp
  progress_meter*.?pp
and it would probably be simpler if I could just commit some changes
for your review. The changes are trivial: s/shared/unique/, and
include the class header (e.g., in 'emit_ledger.hpp', where it seems
like a small price to pay for switching from a heavy shared_ptr to
a lighter unique_ptr).

As for all the shared_ptr members of class BasicValues, I've managed
(in changes that aren't ready to commit) to replace smart pointers
with objects. But this leads to changes like:

diff --git a/basic_values.hpp b/basic_values.hpp
+#include "database.hpp"
...
-    std::shared_ptr<product_database>   Database_;
+    product_database        const       database_;

diff --git a/basicvalues.cpp b/basicvalues.cpp
-#include "database.hpp"
...
 BasicValues::BasicValues(Input const& input)
     :yare_input_              {input}
+    ,policy_                  {}
+    ,database_
+        ("empty for now" // filename
+        ,yare_input_.Gender
+        ,yare_input_.UnderwritingClass
+        ,yare_input_.Smoking
+        ,yare_input_.IssueAge
+        ,yare_input_.GroupUnderwritingType
+        ,yare_input_.StateOfJurisdiction
+        )

and I'm not quite convinced that's ideal. If holding subobjects
directly as const data members, and initializing them in the
ctor-initializer, is good in general, then is it possible to
have too much of this good thing? That would become one honking
huge ctor-initializer.

> but I still think it could be better to start
> with a low-hanging not_null_unique_ptr<> fruit rather than trying to revise
> all pointers used in lmi. This is a worthwhile task, of course, but it will
> take a much longer time to complete and in the meanwhile we still will have
> to use unique_ptr<> even for pointers that can't be null, which is a pity.

You're looking for low-hanging fruit, but I'm asking whether we
should bulldoze the orchard.