Re: [lmi] Pagination anomaly

[Vadim Zeitlin]

On Tue, 6 Feb 2018 12:52:39 +0000 Greg Chicares <address@hidden> wrote:

GC> It's actually easier for me, too. Let me emphasize that when I "merged"
GC> this, I did not run 'git merge': instead, I ran 'git cherry-pick'

 Yes, sorry, I should have been more precise (I need to make a Vim
abbreviation inserting this sentence automatically in my emails, otherwise
I spend too much time typing it out). In any case, either "merge" or
"cherry-pick" (a.k.a. "rebase") is fine. From my point of view, "merge"
(which could be a fast-forward merge in this case, i.e. keeping the history
linear) would be ideal because it would allow me to just run "git branch -d
local-branch" without worrying about losing any changes, but cherry-picking
is not that much worse because I can do "git rebase master local-branch" to
confirm that there are no changes remaining on the branch -- as long as the
commits are the same, Git will do it automatically. The only problematic
(from my point of view) scenario in which commits are applied with changes
as part of the same commit (this isn't a problem if the changes are done as
separate commits later) because then I do get conflicts when running the
rebase.

GC> Then would it make sense to abstract out the pagination code,
GC> and add a unit test like the one you posted yesterday?

 For get_extra_pages_needed() this could be done easily enough. But
render() is not as simple to untangle... If we do decide to keep both
versions of the code, then it would be worth doing it for just
get_extra_pages_needed(), but if we're going to remove it anyhow, then it
wouldn't seem to make much sense.

GC> > GC> BTW, we currently have a header that only defines
GC> > GC> 
GC> > GC> enum enum_output_mode
GC> > GC>     {e_output_normal
GC> > GC>     ,e_output_measure_only
GC> [...and I'd like to...]
GC> > GC> rename everything so that whole entity is self-documenting--e.g.:
GC> > GC> 
GC> > GC> enum oenum_render_or_measure
GC> > GC>     {oe_render
GC> > GC>     ,oe_measure
GC> > GC>     };
GC> [...]
GC> >  No, although I slightly regret the loss of "only" as it underscored that
GC> > rendering also does measuring anyhow.
GC> 
GC> Yes, thanks, that's a good point, so I put 'only' back in.

 Thanks!


GC> This change (recently pushed) raises an incidental question: have we
GC> agreed that it's generally preferable for enumerative 'switch'
GC> statements to write a 'case' for every enumerator, and rely on the
GC> compiler to identify any missing case?

 There are 2 possibilities: the first one is to do this, relying on the
"default" (because included in -Wall) -Wswitch to detect the missing cases.
The second one is to explicitly use -Wswitch-enum (which is not part of
-Wall) to give this warning even if there is a "default" label. The only
advantage of the latter is that it allows to detect the problem during
run-time if it somehow slips through during compilation, but this shouldn't
be possible in lmi, which always uses gcc with -Wall and -Werror. And the
-Wswitch-enum option has a disadvantage of forcing to explicitly list all
the enum elements even if all or many of them are handled in the same way
(trying to build lmi with this option right now results in quite a few
errors). So, for lmi, your proposal indeed seems preferable.

GC> The
GC>     switch(output_mode)
GC> statements in 'pdf_writer_wx.cpp' list every possible 'case' and
GC> also 'default:'.

 I believe that I did it like this for consistency with the other switch
statements in lmi. And, searching for "not found", I do see many similar
occurrences, e.g. switch over display mode in progress_meter_cli.cpp, so,
for once, I remember correctly.

GC> Maybe I should remove 'default:' wherever possible throughout
GC> lmi, once and for all.

 Yes, I think it would be a good (even if not particularly life-changing,
so probably not very urgent) thing to do. It also won't be completely
trivial, some default labels can't be removed easily even if they look
superficially similar to the ones that can. E.g. the switch in
Ledger::SetRunBases() contains one which looks very similar to the one in
pdf_writer_wx.cpp, but the switch there doesn't have cases for several enum
elements (mce_variable_annuity and 3 more mce_xxx_obsolete values).

GC> [...two pagination implementations...]
GC> 
GC> >  The problem is embarrassingly simple, but there is just enough
GC> > comparisons/divisions in it to make it easy to make a mistake in one of
GC> > them... It doesn't make it hard, but it does make it error-prone, and even
GC> > if I'm reasonably certain that I did do it correctly, finally, after
GC> > writing my test and verifying that it passes for all values, it might 
still
GC> > be worth to keep both functions to ensure it doesn't happen again.
GC> > 
GC> >  The main negative of doing it I see is that it will make modifying this
GC> > code more difficult in the future, as it will need to be done in 2 (albeit
GC> > neighboring) places. However I don't believe this code would need to be
GC> > modified often, so it's probably not a big deal.
GC> 
GC> More difficult, for a limited future time only, as long as we decide
GC> to keep only one after the conclusion of acceptance testing.

 So should I take this as the expression of the final decision to get rid
of a separate get_extra_pages_needed()? Or should we still keep it for now,
as that proverbial second chronometer (the difference being that we're not
out in the sea and that if two implementations disagree again, we can
always debug them and see which one of them is right).

 Thanks,
VZ

P.S. Please let me know when you'd like me to stop cc'ing you, AFAICS the
     mailing list works fine now and I reliably receive all your messages
     twice.