Re: [lmi] Why use pointers here?

[Vadim Zeitlin]

On Thu, 5 Apr 2018 01:19:37 +0000 Greg Chicares <address@hidden> wrote:

GC> When I see
GC>   std::unique_ptr<pdf_illustration> pdf_ill;
GC> I start pushing questions and concerns on my mental stack:
GC>   - are we getting pointers from some kind of class factory?
GC>       (answered above--but I had to go looking for the answer)

 Sorry, but why should it matter if a class factory is used or not? It can
be easily seen that it isn't (or is) when the pointer is initialized, but
this seems completely orthogonal...

GC>   - uninitialized pointer: make sure it's initialized before use

 Yes, this is a concern. By declaring it just before the switch statement
and consistently initializing it in every case label, I hoped to assuage
it.

GC>   - is this pointer's ownership going to be transferred elsewhere?

 This is again a valid concern, and it's a pity that standard C++ doesn't
provide an equivalent to boost::scoped_ptr<> which is strictly less
powerful than std::unique_ptr<> -- but sometimes less is more [useful].
But knowing that there is no such class in the standard also implies
knowing that std:unique_ptr<> is used as its replacement and that in most
cases its ownership won't be transferred anywhere -- unless the function
returns this pointer, in which case it clearly will be transferred to the
caller.

 IOW, I'm trying to say that in a function not returning unique_ptr<>, it's
very reasonable to consider that the ownership won't be transferred
anywhere by default and be very upset if it is, without a comment warning
about this happening beforehand.

GC> Finally, backtracking one last time, I see that there are no move()
GC> shenanigans, and no reset(), swap(), or release().

 I.e., again, all these methods should be used only in exceptional cases
and you really shouldn't have to worry about them by default.

GC> IOW, I have a panic attack whenever I see a smart pointer: it opens
GC> too many possibilities that just don't exist for plain objects on
GC> the stack. I guess this is effortlessly transparent to you

 It's certainly less transparent than a plain object, but OTOH I don't
expect the author of the code to intentionally trick me neither, so when I
see a local unique_ptr<> I just assume it's going to be used to store some
heap-allocated object until the end of the scope -- unless warned otherwise
by a prominent comment. Maybe it's just my trusting nature, of course, but
OTOH I really don't think you can ever be sure that the code doesn't
contain any surprises -- even a plain object could be std::move()'d from,
couldn't it? Paraphrasing a well-known saying, the only reasonable goal of
code review is to protect against Murphy, not Machiavelli.

GC> For me at least, it's much clearer to read this:
GC> 
GC>     switch(ledger.ledger_type())
GC>         {
GC>         case mce_ill_reg:
GC>             pdf_illustration_regular (ledger).render_all(output);
GC>             break;
GC>         case mce_nasd:
GC>             pdf_illustration_nasd    (ledger).render_all(output);
GC>         ...

 OTOH changing the code to do something else than just calling render_all()
is more difficult now and, at least for me, this does make understanding it
more difficult because I can't help wondering why do we have all these
duplicated function calls, so I'd spend time carefully checking that
they're really all the same and that I'm not missing something...

GC> > But maybe what we really want is a helper function like
GC> > this:
GC> > 
GC> > template<typename T>
GC> > void create_illustration(Ledger const& ledger, fs::path const& output)
GC> > {
GC> >     T pdf_ill{ledger};
GC> >     pdf_ill.render_all(output);
GC> > }
GC> At first, I thought "pdf_ill{ledger}" was a zsh glob representing some
GC> complexity that would need to be translated, and I didn't feel up to
GC> that challenge, so I made commit 6586cab0.
GC> 
GC> Only now that the strong medication for that tooth extraction is
GC> wearing off do I realize that '{}' is C++1x initialization syntax.
GC> But I haven't recovered enough to see where this helper would go.

 I thought it would be just a simple helper function. Or it could also be a
static member of pdf_illustration, but in this case we'd have to write
pdf_illustration::render<pdf_illustration_xxx>() and this would be a bit
redundant.

 Just in case it could be useful, here is the trivial patch adding this
function, in order to avoid duplicating render_all() calls and, maybe, make
the code a bit more clear:

---------------------------------- >8 --------------------------------------
diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
index 9b7a18f4f..56e895d9d 100644
--- a/ledger_pdf_generator_wx.cpp
+++ b/ledger_pdf_generator_wx.cpp
@@ -2916,6 +2916,20 @@ class ledger_pdf_generator_wx : public 
ledger_pdf_generator
   private:
 };

+namespace
+{
+
+// Helper of write() below: writes the illustation of the specified type T
+// using data from the given ledger to the provided output path.
+template<typename T>
+void do_write(Ledger const& ledger, fs::path const& output)
+{
+    T pdf_ill{ledger};
+    pdf_ill.render_all(output);
+}
+
+} // Unnamed namespace.
+
 void ledger_pdf_generator_wx::write
     (Ledger const& ledger
     ,fs::path const& output
@@ -2926,16 +2940,16 @@ class ledger_pdf_generator_wx : public 
ledger_pdf_generator
     switch(ledger.ledger_type())
         {
         case mce_ill_reg:
-            pdf_illustration_regular         (ledger).render_all(output);
+            do_write<pdf_illustration_regular>(ledger, output);
             break;
         case mce_nasd:
-            pdf_illustration_nasd            (ledger).render_all(output);
+            do_write<pdf_illustration_nasd>(ledger, output);
             break;
         case mce_group_private_placement:
-            pdf_illustration_reg_d_group     (ledger).render_all(output);
+            do_write<pdf_illustration_reg_d_group>(ledger, output);
             break;
         case mce_individual_private_placement:
-            pdf_illustration_reg_d_individual(ledger).render_all(output);
+            do_write<pdf_illustration_reg_d_individual>(ledger, output);
             break;
         case mce_prospectus_obsolete:                 // fall through
         case mce_offshore_private_placement_obsolete: // fall through
---------------------------------- >8 --------------------------------------

 Regards,
VZ