Re: [lmi] Unit test for PDF get_extra_pages_needed() function

[Greg Chicares]

On 2018-02-06 22:22, Vadim Zeitlin wrote:
> 
>  I've extracted the logic of one of the two PDF pagination functions (the
> one which was easier to refactor) in a new get_needed_pages_count() in
> miscellany.hpp and added a simple unit test for it checking for all the
> problematic cases encountered so far, please see
> 
>       https://github.com/vadz/lmi/pull/64
> 
>  I'm not sure if this function is really appropriate for miscellany.hpp as
> it's very PDF-specific and is unlikely to be reused elsewhere. OTOH it is a
> pure function similar to the other ones already present in this header and
> miscellany_test.cpp looked like the most appropriate place for the new unit
> test as nothing else seemed to fit and it doesn't seem to merit a whole new
> test neither.

That's the perfect place for it. And if we add a capability to write
flat-text files as an easier-to-test adjunct to PDFs, then it's reusable
in that non-PDF context. We could also use it to help print paginated
rate tables (of which we have hundreds) to flat-text files.

I see how get_needed_pages_count() evolved--it's basically transplanted
from PDF-specific code--yet I wonder whether it would be more useful in
other cases if it became a class that calculated and returned other data:
 - number of pages needed, as at present
 - number of data-rows to write on pages other than the last
 - number of data-rows to write on the last page
 - number of lines of output (including blank lines) on such pages
because all of those things are readily available. However, I haven't
studied the context from which it was transplanted, or looked at the
other function you mentioned that does something similar in a different
way, so this is just speculation.

What are the actual preconditions, and where are they enforced?
The header:
  /// Preconditions: total_rows > 0 && rows_per_page >= rows_per_group > 0
seems to list more preconditions than the implementation:
  // The caller must check for this precondition because this function is too
  // low-level to be able to handle it correctly, e.g. it can't even use the
  // appropriate error message.
  LMI_ASSERT(rows_per_page >= rows_per_group);
Does the comment in the implementation mean that any code that calls
this from upstream must repeat the assertion itself? Are you saying
that an assertion upstream could provide a more informative diagnostic,
like "Not enough room for even one group in file 'foo.pdf'"?