Re: [lmi] Header pagination PR

[Vadim Zeitlin]

On Tue, 18 Sep 2018 20:57:15 +0000 Greg Chicares <address@hidden> wrote:

GC> >  Moreover, I also think that we might do something similar for the footers
GC> > now: they have less variability than the headers, which allowed me to
GC> > handle them in the code without too much trouble, but I think it would be
GC> > better to define the footers entirely in the .mst files too, rather than
GC> > doing it partially there and partially in the headers. Please let me know
GC> > if you agree and I'll make the necessary changes (it is not urgent, of
GC> > course).
GC> 
GC> I do agree, and it's not urgent. Can you do this entirely in '*.mst',
GC> or are code changes needed? If it's all '*.mst' work, then it shouldn't
GC> conflict with anything I plan to do this week. Or, if it needs only a
GC> few code changes, it might not be too difficult to merge soon. Or we
GC> can put it off until later.

 I think it would be better to put it off until (slightly) later because it
does require some relatively extensive code changes as well: while I can
add support for <footer> almost entirely at .mst level (which still doesn't
guarantee that there will be no conflicts BTW, e.g. the FINRA renaming
would have wreaked havoc with such change too), I can't really remove the
existing support for footers at the code level without changing the latter.

 So I'd prefer for you to do everything you want to do with this code first
and then make my changes.


GC> > GC> 71dc467e Pass the illustration and interpolator to the page ctor
GC> > GC> 
GC> > GC> In uncommitted work that I've put aside for the moment, I believe
GC> > GC> I can demonstrate that the 'illustration_' member can provide
GC> > GC> everything we've been passing as arguments to so many functions:
GC> > GC>   {Ledger, interpolator, pdf_writer}
GC> > 
GC> >  It could, but I'd like to keep the separation between the illustration 
and
GC> > the interpolator as I feel it could be useful to not use the same object
GC> > for both in the future (e.g. we could write some tests verifying that the
GC> > interpolation works correctly without using any illustration objects). And
GC> > I think it's generally preferable to reduce dependencies between objects 
by
GC> > passing the object U (user) the object D (dependency) that it needs
GC> > directly instead of passing the object U the object I (intermediate) which
GC> > it can use to get D from, as it reduces long-distance coupling between the
GC> > different components. Here the difference is not very big and the entire
GC> > subsystem is sufficiently small that it might not be a concern at all
GC> > (although small things have tendency to grow with time), but I don't think
GC> > it's really that burdensome to pass these objects as arguments neither and
GC> > I'd prefer not to change that.
GC> 
GC> Let me give that some more thought. I do agree that U and D should be
GC> kept separate. What I'm thinking is that
GC>   U(D d, I i) : d_(d), i_(i) {...}
GC>   U::foo() {d_.bar();}
GC> is one way of doing it,

 Note that this is not quite what I advocate. I rather want the caller of
U::foo() to do whatever is necessary to provide foo() with D, that it
really needs. I.e. if U knows about I anyhow, this doesn't make much sense.
I'd rather lift the dependency on I one level up, where it is implicit
anyhow, and let U to be happily ignorant about it.

GC> and
GC>   U(D d) : d_(d) {...}
GC>   U::foo() {d_.get_i().bar();} // U has a d_ but no i_

[note this seems to inverse the meanings of "D" and "I" given above]

GC> is another way that has similar advantages.

 I disagree here. This has a different advantage, namely that you have
fewer objects to pass around, but it loses the advantage of U not having to
know anything about get_x().

GC> But I'll take a fresh look tomorrow and compare HEAD with the work
GC> I had put off to the side. We've been working toward the same goal,
GC> and it might turn out that you've already completely achieved it.

 If the goal is moving state into the page classes, then I indeed have
partially accomplished it. But I have no special motivation to go further
along this road, I only did what I had done because the alternative was too
unwieldy, but current code looks fine to me and, again, I'd leave it be
because it's not at all obvious to me that the alternative would be better:
it would be slightly shorter, but also have tighter coupling, and this is
usually not a good trade-off to make.

GC> > GC> 'ledger_pdf_generator_wx.cpp': 92 hunks of changes, so this will take 
a
GC> > GC> little longer.
GC> > 
GC> >  Maybe reviewing the changes commit-by-commit will be simpler than looking
GC> > at all of them at once. E.g. 71dc467eb0a3ff0b83b81f3ded4e1c21934604a8
GC> > contains a lot of changes, but they're trivial as this commit is a pure
GC> > refactoring and doesn't actually change anything. OTOH the following
GC> > commits b2ec7acde109641d06fd0c7ee774a32bb9219fc4 and
GC> > 1c398a864f52103b63276f2ad13ad89e20f03bc1 contain fewer, but less trivial,
GC> > changes.
GC> 
GC> Simpler, yes, but OTOH that would be cheating.

 Hmm, I don't see it all in this way. I also definitely do it myself when
reviewing your changes and I'm not ashamed at all of making the reviewing
easier by doing it.


[inheriting ctors]
GC> Oh. I remember seeing that term in my slowly-progressing studies of
GC> modern C++, but I didn't take much notice because I didn't understand
GC> its motivating purpose.

 It's just a detail and not at all as important as the major changes in the
recent C++ versions (move semantics, auto, ...), but I like inheriting and
delegating ctors in C++11 a lot as they seem like a very natural way to
avoid having to write some tedious and so clearly unnecessary code that we
had to write before.

 Regards,
VZ