Re: [lmi] Group quotes, part deux

[Vadim Zeitlin]

On Wed, 18 May 2016 11:15:39 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2016-05-11 13:01, Vadim Zeitlin wrote:
GC> > On Sun, 8 May 2016 22:16:25 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC> > GC> Later, in output_document_header(), I have a different design 
question:
GC> > GC> > ...
GC> > GC> > GC> Is there good reason not to treat everything the same way, e.g.,
GC> > GC> > GC> replacing the above block by
GC> > GC> > GC> 
GC> > GC> > GC>     fields.push_back(extra_summary_field({"Plan Type", 
report_data_.product_}));
GC> > GC> > GC> +   fields.push_back(extra_summary_field({"Product", 
plan_type_}));
GC> > GC> > GC> +   fields.push_back(extra_summary_field({"Effective Date", 
report_data_.effective_date_}));
GC> > GC> > GC> 
GC> > GC> > GC> and letting the loop for "parsed" input handle everything? I 
don't
GC> > GC> > GC> want to stomp on your design without asking, lest I destroy 
something
GC> > GC> > GC> valuable that I just don't happen to notice.
GC> > GC> > 
GC> > GC> >  No, I don't think this was done intentionally at all, AFAIR the 
extra
GC> > GC> > summary fields were added later and I just didn't realize I could 
refactor
GC> > GC> > the existing code to handle both the built-in and the extra fields 
in the
GC> > GC> > same way. Looking at it right now, this definitely looks like a 
good idea.
GC> > GC> > Should I do it or leave it to you as you're already in this code?
GC> > GC> 
GC> > GC> Please do it, as you're the original author so it's easier for you.
GC> > GC> I've completed the changes I intended for now, so I'll hand this
GC> > GC> source file over to you.
GC> > 
GC> >  I'll do this slightly later

 I've actually decided to do this right now too, for the same reasons you
give below: while the change is trivial, it would still be better to test
it now while you're testing this code anyhow, than postpone it for later.

GC> [...]
GC> > GC> ...so let me ask you to address this globally as you like along
GC> > GC> with any other C++11-ization that seems advisable.
GC> > 
GC> >  As mentioned before, I'd prefer to modernize the entire code base at 
once,
GC> > as otherwise it's going to be jarring to see "NULL" here and "nullptr"
GC> > there, lmi::uncopyable<> here and "= delete" there and so on. But doing it
GC> > right now would certainly create many conflicts with the still outstanding
GC> > patches, so I'd like to wait until you can apply at least those of them
GC> > that you plan to apply.
GC> 
GC> Would you mind addressing this particular part now, separately from the
GC> global modernization? Here's why I ask. This code in
GC>   group_quote_pdf_generator_wx::output_document_header()
GC> already uses emplace_back(), along with two other methods to assemble the
GC> same set of headers, so this isn't just modernization--it's restructuring
GC> for consistency and simplicity. And we're testing group quotes thoroughly
GC> now, so testing this change separately later will take extra effort that
GC> can be avoided by doing it now.

 Actually after thinking about this again, I realized that there is no
point in using emplace_back() here, sorry for misleading you initially. The
reason for preferring emplace_back() is that it can construct the object
directly in the memory managed by the vector instead of constructing a
temporary and then copying it there. But in this particular case we're
explicitly creating this temporary ourselves and then emplace_back()
constructs the real object using the (implicitly generated by the compiler)
move ctor of extra_summary_field class. So this is exactly the same as
using push_back() which will construct a temporary implicitly and then use
move ctor to initialize the new vector element from it.

 However push_back() is simpler to use because it supports uniform
initialization and so we can write just push_back({name, value}) instead of
having to explicitly use the class name as we must do with emplace_back()
because it does not support uniform initialization, so it becomes
emplace_back(extra_summary_field{name, value}). It would be possible to add
a ctor from 2 strings to extra_summary_field and then write just
emplace_back(name, value), but I'm not sure if it's worth the trouble.

 Of course, in this particular case the performance difference between
different approaches is completely unnoticeable anyhow (in the worst case
we're copying two strings containing a dozen characters an extra time), but
it's a bit bothersome that there doesn't seem to be any way to have optimal
performance *and* simple code. I think push_back({name, value}) is the best
compromise because it avoids writing the ctor by hand (which not only saves
the two lines needed to do it now, but avoids the need to update this code
later if any new fields are added), is almost optimally efficient (and
maybe even optimally so, I'd need to run benchmarks to be sure) and is
quite clear on the calling side.

 So I've done this for now, but it would, of course, be simple to change if
you prefer another approach. Please let me know if you do.

 For now my changes are at https://github.com/vadz/lmi/pull/33, any
feedback is welcome, as always.

 Thanks,
VZ