Re: [lmi] Preliminary emit_ledger code refactoring

[Vadim Zeitlin]

On Wed, 29 Jul 2015 17:37:18 +0000 Greg Chicares <address@hidden> wrote:

GC> >  The real impetus behind this change is the need to store as members
GC> > something that is not passed as arguments to these functions
GC> 
GC> I'm not sure I understand. Are we really talking about the state of the
GC> PDF generator?

 Yes, I do. In this case "state" is actually all the data emitted so far
and it needs to be stored from pre_emit_ledger() through multiple calls to
emit_ledger() and until it can finally be output in the last call to
emit_ledger() with a composite ledger or end().

GC> Or is there some required argument beyond those used today,
GC> which will need to be added in the future?

 No, nothing will need to be added.

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

 Yes, exactly. It is completely orthogonal to the headers/footers (which
correspond to the methods of ledger_single_format_emitter). Maybe the name
of this class is insufficiently clear but I couldn't come up with anything
better...

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

 Oops, now that you mention it (again?), I think you did say it, but
currently I sum up the numbers in the PDF generation code. I guess I just
need to take the values from the corresponding fields of the [invariant
part of the] composite ledger instead, right?

 Notice that this won't materially change anything for the PDF code, as it
will still have to delay output until the totals are available, i.e. the
very end of emission, because it's not possible to go back to the first
page to output them at the end if the document has more than one page with
wxPdfDocument API.

GC> There's one more detail I wanted to ask about. In this code:
GC> 
GC>         if(emission & mce_emit_pdf_to_printer)
GC>             {
GC>             emitters_.push_back(new ledger_emitter_pdf_to_printer());
GC>             }
GC>         if(emission & mce_emit_pdf_to_viewer)
GC>             {
GC>             emitters_.push_back(new ledger_emitter_pdf_to_viewer());
GC>             }
GC>         ...
GC> 
GC> is there a reason to manage dynamic allocation ourselves with new() and
GC> delete() and a vector of pointers:
GC>   std::vector<ledger_single_format_emitter*> emitters_;
GC> ? Could we instead do
GC>   std::vector<ledger_single_format_emitter> emitters_;  // no pointers
GC>   emitters_.push_back(ledger_emitter_pdf_to_printer()); // no new()
GC> and let the vector delete its contents when it goes out of scope?

 This won't work at all as it would result in slicing. We really need to
allocate the objects dynamically to be able to treat them polymorphically.
And the only way to avoid managing the memory manually ourselves is to use
a value-like wrapper such as boost::shared_ptr<>. In C++11 I would have
used std::unique_ptr<> without thinking twice, but in C++98 I decided to
use raw pointers because shared_ptr<> does have some overhead (unlike
unique_ptr<>) and the code seems "obviously" safe: the vector elements are
allocated in the ctor, the vector never changes and its elements are freed
in the dtor. Still, maybe I should have used shared_ptr<> instead as the
overhead is probably insignificant and it would ensure memory safety even
if the code changes and becomes more complicated in the future...

 Of course, personally I'd rather make it _simpler_ by disallowing to
specify a bit mask of mce_emit_xxx values, as I wrote in
http://lists.nongnu.org/archive/html/lmi/2015-07/msg00000.html and if this
were done we could just use boost::scoped_ptr<> for the single pointer that
would need to be stored (but we can't store scoped_ptr<> in a std::vector<>).


GC> > while I know of people who use C++ without exceptions, templates or
GC> > deep object hierarchies, I've never heard about any rules against
GC> > using simple objects.
GC> 
GC> Think of it this way--imagine that you have written some well-tested
GC> code that uses the printf() family extensively, and I propose replacing
GC> it with iostreams. My version is twice as long, but I can certainly say
GC> that it's typesafe. Do you apply my changes with no hesitation at all?

 Well, the superiority of iostreams compared to printf() is still a matter
of quite some debate (in which I'm firmly on printf() side), so, no, I
wouldn't. But replacing related functions with objects and method calls
seemed like a much more settled question to me, which is why I was so
surprised by your reservations about doing it.

 I do understand hesitations about replacing the known well-working code
with something else, however these changes are pure refactorings which, I
think, can be relatively easily verified to be correct. And this code has
tests which continue to pass which provides an extra safety.

 Please let me know if you have any other questions or suggestions for
simplifications (currently I remain with the idea that I should get rid of
the "_impl" class at the price of increasing physical coupling),
VZ