Re: [lmi] Reference lifetime extension

[Vadim Zeitlin]

On Thu, 11 Oct 2018 20:31:12 +0000 Greg Chicares <address@hidden> wrote:

GC> I had saved this little change off to the side, and was thinking of
GC> committing it, but luckily it faulted when I tested it repeatedly:
GC>   Unhandled exception: page fault on execute access to 0x068a9050 in 32-bit 
code (0x068a9050).
GC> so I know it's wrong; but why, exactly?
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/ledger_pdf.cpp b/ledger_pdf.cpp
GC> index d3eb99c4..c83a0d3b 100644
GC> --- a/ledger_pdf.cpp
GC> +++ b/ledger_pdf.cpp
GC> @@ -94,6 +94,6 @@ std::string write_ledger_as_pdf(Ledger const& ledger, 
fs::path const& filepath)
GC>      if(0 == scaled_ledger.GetLedgerInvariant().ScalePower())
GC>      scaled_ledger.AutoScale();
GC> -    auto const pdf = ledger_pdf_generator::create();
GC> -    pdf->write(scaled_ledger, pdf_out_file);
GC> +    ledger_pdf_generator const& pdf = *ledger_pdf_generator::create();
GC> +    pdf.write(scaled_ledger, pdf_out_file);
GC>  
GC>      return pdf_out_file.string();
GC> 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

 Sorry if this is a trick question, but this seems relatively obvious to
me: the shared_ptr<> returned by ledger_pdf_generator::create() is
destroyed at the end of the statement, as there is no const reference bound
to it that could possibly extend its life-time. Instead, you have a const
reference set to the result of its operator*(), but this operator doesn't
return a temporary object, so rules about extending its life-time don't
apply at all.

 The result is, of course, undefined behaviour because you keep a reference
equal in value to the pointer which becomes dangling after the shared_ptr
destruction.

GC> Here, create() is a callback to this member function:
GC> 
GC>     static std::shared_ptr<ledger_pdf_generator> do_create()
GC>         {
GC>         return std::make_shared<ledger_pdf_generator_wx>();
GC>         }
GC> 
GC> and a postcondition of std::make_shared<>() is that get() is non-null,
GC> so I infer that the lifetime of the temporary shared_ptr isn't extended
GC> to the line after create() is called.

 Yes.

GC> And I reason that this is because C++17 (N4659) says in 15.2
GC> [class.temporary]:
GC> 
GC> | [6] The temporary to which the reference is bound or the temporary
GC> | that is the complete object of a subobject to which the reference is
GC> | bound persists for the lifetime of the reference except:
GC> [...]
GC> | [6.2] The lifetime of a temporary bound to the returned value in a
GC> | function return statement (9.6.3) is not extended; the temporary is
GC> | destroyed at the end of the full-expression in the return statement.
GC> 
GC> ...so [15.2/6.2] is the exact explanation, right?

 No, I don't think it applies here. If you assigned the return value of
create() to a const reference, it would be just fine, i.e. this code

        auto const& pdf = ledger_pdf_generator::create();
        pdf->write(scaled_ledger, pdf_out_file);

is still correct and marginally more efficient than the current version as
it avoids shared_ptr copy ctor (which is not at all trivial as it must be
MT-safe). Of course, it's exactly the same as

        ledger_pdf_generator::create()->write(scaled_ledger, pdf_out_file);

which might perhaps be more obviously correct.

GC> Somehow I still have great difficulty seeing at a glance why the patch
GC> above is wrong. I guess I just try to avoid shared_ptr in general, and
GC> assume that it always lengthens the lifetime of the object pointed to,
GC> and I need to bear in mind that when a function returns a temporary
GC> shared_ptr, that shared_ptr will expire at the next semicolon, and any
GC> reference I had formed to it will be left dangling.

 Yes, exactly.

 Avoiding using shared_ptr<> without real reason is a good idea, of course,
but as long as you do use it, you must not use the pointer (or reference,
which is the same thing) it contains directly, this is really error-prone
and is only ever necessary when interfacing with legacy code.

 Please let me know if you have any remaining questions about this,
VZ