Re: [lmi] Why use pointers here?

[Greg Chicares]

On 2018-04-02 13:27, Vadim Zeitlin wrote:
> On Mon, 2 Apr 2018 13:19:49 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-03-31 16:18, Vadim Zeitlin wrote:
> GC> > 
> GC> >  [...] please see https://github.com/vadz/lmi/pull/78
> GC> BTW, why use pointers here?
> GC> 
> GC> @@ -2942,16 +2929,16 @@ void ledger_pdf_generator_wx::write
> GC>      switch(z)
> GC>          {
> GC>          case mce_ill_reg:
> GC> -            pdf_ill = std::make_unique<pdf_illustration_regular>(ledger, 
> output);
> GC> +            pdf_ill = std::make_unique<pdf_illustration_regular>(ledger);
> GC>              break;
> GC> ...
> GC> -    pdf_ill->render_all();
> GC> +    pdf_ill->render_all(output);
> GC> 
> GC> AFAICT, classes like pdf_illustration_regular aren't "factories"
> GC> that only return smart pointers, so couldn't we instead write:
> GC> 
> GC>   pdf_illustration_regular>(ledger).render_all(output)
> GC> 
> GC> and similarly for the other classes?
> 
>  This would require repeating the render_all() call for all the 4 cases,

True; but IMO that repetition is simpler. When I see
  std::unique_ptr<pdf_illustration> pdf_ill;
I start pushing questions and concerns on my mental stack:
  - are we getting pointers from some kind of class factory?
      (answered above--but I had to go looking for the answer)
  - uninitialized pointer: make sure it's initialized before use
  - is this pointer's ownership going to be transferred elsewhere?
Then there's some code that initializes it--okay, it's initialized
along every path that doesn't throw. Then we call render_all()
through the pointer. Now I see that the purpose was to construct
an instance of the appropriate derived class and later call
render_all() on it. Finally, backtracking one last time, I see that
there are no move() shenanigans, and no reset(), swap(), or release().

IOW, I have a panic attack whenever I see a smart pointer: it opens
too many possibilities that just don't exist for plain objects on
the stack. I guess this is effortlessly transparent to you, for some
reason I'll never understand--just as I can't understand how you
manage to make a basketball go through a hoop.

For me at least, it's much clearer to read this:

    switch(ledger.ledger_type())
        {
        case mce_ill_reg:
            pdf_illustration_regular (ledger).render_all(output);
            break;
        case mce_nasd:
            pdf_illustration_nasd    (ledger).render_all(output);
        ...

First level: This write() function does something that varies by
ledger type. That's a natural requirement, and we're naturally going
to use 'switch'. That's the only control construct, and it's used in
the normal textbook fashion, so there's no stack of worries to push
now and pop later.

Second level: In each case, construct an instance of the corresponding
derived class and call the same render_all() function on that instance.
The 'ledger' and 'output' arguments are identical in each case, and
indentation makes the sameness obvious. It's just a single statement
whose purpose and whose correctness are clear. There's no possible
question of lifetime, ownership, or (un)initialization.

> But maybe what we really want is a helper function like
> this:
> 
> template<typename T>
> void create_illustration(Ledger const& ledger, fs::path const& output)
> {
>     T pdf_ill{ledger};
>     pdf_ill.render_all(output);
> }
At first, I thought "pdf_ill{ledger}" was a zsh glob representing some
complexity that would need to be translated, and I didn't feel up to
that challenge, so I made commit 6586cab0.

Only now that the strong medication for that tooth extraction is
wearing off do I realize that '{}' is C++1x initialization syntax.
But I haven't recovered enough to see where this helper would go.
Is it just a replacement for ledger_pdf_generator_wx::write(),
which switches on type through a template parameter rather than
a switch statement?