Re: [lmi] Reference lifetime extension

[Vadim Zeitlin]

On Fri, 12 Oct 2018 01:17:06 +0000 Greg Chicares <address@hidden> wrote:

GC> On 10/11/18 11:58 PM, Vadim Zeitlin wrote:
GC> > On Thu, 11 Oct 2018 23:29:12 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> >  Also, there is a fact that the latest master doesn't work at all for me
GC> > when building with MSVS, but this is only an easy to fix typo, could you
GC> > please apply this patch to correct it?
GC> [...]
GC> > diff --git a/main_wx.cpp b/main_wx.cpp
GC> [...]
GC> >  LMI_FORCE_LINKING_EX_SITU(group_quote_pdf_generator_wx)
GC> > -LMI_FORCE_LINKING_IN_SITU(pdf_command_wx)
GC> > +LMI_FORCE_LINKING_EX_SITU(pdf_command_wx)
GC> >  LMI_FORCE_LINKING_EX_SITU(progress_meter_wx)
GC> 
GC> Certainly. I'll also remove the similar defect in 'main_wx_test.cpp'.
GC> Those changes are already committed, but I don't want to run the
GC> entire battery of (more-frequent-than) nychthemeral tests tonight,
GC> so I'll push them tomorrow.
GC> 
GC> Do you see any easy way to cause such defects to produce an error
GC> with gcc, so that they'll be self-identifying in future?

 All I can think of is to replace the two LMI_FORCE_LINKING_XXX macros with
a single LMI_FORCE_LINKING and force predefining either LMI_FORCE_IN_SITU
or LMI_FORCE_EX_SITU before including this file (i.e. give an #error if
neither is defined). This would make this kind of error impossible, but
would be (slightly) more verbose and arguably even less clear. Not sure if
it's really worth it, especially because it would still be possible to just
forget to use these macros in the first place and this can't be detected
without actually trying to link with MSVC linker.


GC> > Getting rid of shared_ptr<> is a good thing (and I admit that I have
GC> > absolutely no idea why had I used it here in the first place),
GC> 
GC> I suppose you copied the progress_meter callback mechanism for
GC> group quotes, and then transitively for PDF illustrations.

 Yes, this looks like a likely explanation. Whenever I can find something
similar in the existing code base (with the exception of its legacy parts),
I try to make new code as similar to the existing lmi code as close to it
as makes sense.

GC> > but I would have preferred just replacing it with unique_ptr<>
GC> > instead of completely removing ledger_pdf_generator class.
GC> 
GC> Would unique_ptr<> work for progress_meter, and, similarly,
GC> the "group_quote_pdf_gen" classes? Class progress_meter is so
GC> old that perhaps the only smart pointers available at the time
GC> were boost::shared_ptr and std::auto_ptr.

 AFAICS unique_ptr<> would work perfectly fine for both of them. But I
still wonder why had shared_ptr<> been used originally, even auto_ptr<>
would have worked for this.


GC> I'd much rather use no smart pointers at all,

 I think unique_ptr<> is perfectly fine and have absolutely no qualms about
using it. This is not the case for shared_ptr<>, but for reasons which have
nothing to do with it being possibly null (rather, it's because it
introduces non-local relationships in the code that make it difficult to
reason about, just like global variables).

GC> because pointers can be null and it's tiresome to check that they
GC> aren't. Is it even possible to write "smart reference" classes that
GC> can't ever contain a null value?

 Not without sacrificing movability because being null is the only
reasonable possibility for an object that was just moved from. If you don't
need movability, then you could have some non_null_scoped_ptr, but in
practice it's not very useful because you can just replace it with an
object instead and you can't neither initialize it conditionally nor return
from a function.

 The core guidelines recommend using not_null<T*> which should appeal to
you as it avoids any mention of smart pointers (which still need to own the
object, however). I don't like it that much because if you accept the
guideline that functions not changing ownership should accept pointers only
as T*, it means having a lot of .get() calls in the code, but maybe you
wouldn't mind it that much.

 Finally, there is a natural idea of using not_null<unique_ptr<T>> but
there seems to be a problem with this in the most widely implementation of
not_null, see https://github.com/Microsoft/GSL/issues/89 and the proposed
fix at https://github.com/Microsoft/GSL/pull/675

 I'm all but certain that you're not going to want to incorporate the
entire Microsoft GSL implementation anyhow, but perhaps we could write our
own not_null<unique_ptr<T>> using the information above (and yes, I'd be
willing to do this, please let me know if I should).

 Regards,
VZ