Re: [lmi] Preliminary emit_ledger code refactoring

[Greg Chicares]

On 2015-07-29 14:14, Vadim Zeitlin wrote:
> On Wed, 29 Jul 2015 10:14:23 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2015-07-16 20:41, Vadim Zeitlin wrote:
> GC> [...]
> GC> >         Use ledger_emitter class instead of [pre_]emit_ledger() 
> functions.
> GC> 
> GC> I'm studying this patchset now, and I'm stuck on this particular patch.
> GC> 
> GC> Previously the ledger-emitter implementation was just two functions:
> GC>   double LMI_SO pre_emit_ledger(...);
> GC>   double LMI_SO     emit_ledger(...);
> GC> 
> GC> It might seem sensible enough to combine those into one single class
> GC>   class LMI_SO ledger_emitter
> GC> because, after all, some of the arguments of those two functions were
> GC> the same and may as well be stored as members.
> 
>  The real impetus behind this change is the need to store as members
> something that is _not_ passed as arguments to these functions

I'm not sure I understand. Are we really talking about the state of the
PDF generator? Or is there some required argument beyond those used today,
which will need to be added in the future?

> GC> OTOH, this patch adds
[...numerous classes...]
> GC> all of which makes it much more complex.
> 
>  I think it depends on the point of view.

Thank you for the detailed rationale. While I'm pondering it, let me ask
some questions about fine points.

>  Next, ledger_emitter_single_output_base is really just a way to reuse code
> and avoid duplicating these 2-3 lines which are the same for the 3 classes
> (2 existing ones and a new one).

You have
  ledger_emitter::start()
for the old pre-emission step (PrintRosterHeaders(), e.g.), and
  ledger_emitter::end()
for a post-emission step that hasn't been required in the past. Let me
refer to those things here as "headers" and "footers", which are both
optional and surround a non-optional "body".

Premium quotes require both a header and a footer. Would that necessitate
a new intermediate class? Would we wind up with a hierarchy like...
  class body              // body only, no header or footer possible
  class body_with_header // but no footer
  class body_with_footer // but no header
  class body_with_header_and_footer
which would grow exponentially if we ever add another optional component
or find a case where the "body" is actually optional?

Or is class ledger_emitter_single_output_base instead to be understood as
a ledger_single_format_emitter that actually makes use of a tsv_filepath,
as distinguished from those that ignore it?

> GC> > They are needed because the
> GC> > PDF generation can't be done in a single pass, if only because of the 
> page
> GC> > numbers (but also for a few other reasons), and so the code needs to 
> keep
> GC> > its state between the calls to emit_ledger().
[...]
>  There is the number of pages and also the totals which can only be
> computed at the end, so the generation just can't be done in a single pass.
> Because of this, all the data is just stored initially and then everything
> is generated at once when end() is called.

It's too bad I got pulled away for other tasks, so I can't remember whether
I've mentioned this before...but the 'composite' is a summary whose purpose
is to provide totals of all data.

There's one more detail I wanted to ask about. In this code:

        if(emission & mce_emit_pdf_to_printer)
            {
            emitters_.push_back(new ledger_emitter_pdf_to_printer());
            }
        if(emission & mce_emit_pdf_to_viewer)
            {
            emitters_.push_back(new ledger_emitter_pdf_to_viewer());
            }
        ...

is there a reason to manage dynamic allocation ourselves with new() and
delete() and a vector of pointers:
  std::vector<ledger_single_format_emitter*> emitters_;
? Could we instead do
  std::vector<ledger_single_format_emitter> emitters_;  // no pointers
  emitters_.push_back(ledger_emitter_pdf_to_printer()); // no new()
and let the vector delete its contents when it goes out of scope?

> while I know of people who use C++ without
> exceptions, templates or deep object hierarchies, I've never heard about
> any rules against using _simple_ objects.

Think of it this way--imagine that you have written some well-tested
code that uses the printf() family extensively, and I propose replacing
it with iostreams. My version is twice as long, but I can certainly say
that it's typesafe. Do you apply my changes with no hesitation at all?