[lmi] Control flow in page_with_tabular_report::render()

[Greg Chicares]

Vadim--Can you help me understand the flow of control in
page_with_tabular_report::render()? Filtering out everything
that doesn't seem to affect control flow:

        // The table may need several pages, loop over them.
        for(int year = 0; year < year_max; )
            for(;;)
                {
                ++year;
                if(year == year_max)
                    {
                    // We will also leave the outer loop.
                    break;
                    }
                if(year % rows_per_group == 0)
                    {
                    if(pos_y > page_bottom - rows_in_next_group*row_height)
                        {
                        [base class's render() called here]
                        break;
                        }
                    }
                }
            }

What does the middle 'for' (i.e., "for(;;)" accomplish?

This comment
                    // We will also leave the outer loop.
apparently means that 'break;' exits the inner "for(;;)" loop,
and, because "if(year == year_max)" is true, then the outer
loop's "; year < year_max;" condition is false, so it's also
exited. But why can't this all be done more clearly in a single
'for' loop?

I looked back at this change:
  git show 74872e0
where "for(;;)" was introduced. It was originally:
    for(; year < year_max; ++year)
but even there the logic was:

        for(int year = 0; year < year_max; ++year)
            {
            int pos_y = render_or_measure_fixed_page_part ...
            for(; year < year_max; ++year)
                if...
                    if...
                        break;

and the double for-loop confuses me: is the intention just
to set 'pos_y' iff 0==year?

Can this be replaced by simpler logic? What do you think of
the tentative attempt presented in the patch below? I'm not
satisfied with it because I'd like to get rid of all 'break;'
statements, but...is this function recursive? This function
  page_with_tabular_report::render()
calls
  numbered_page::render()
but that class doesn't declare such a function, so...is it
the intention to call
  page_with_footer::render()
(e.g., to delegate some low-level work), or to call
  page_with_tabular_report::render()
recursively?

I'm hoping that, if I can comprehend this function deeply,
I'll be able either to implement it in terms of something
akin to get_needed_pages_count(), or at least to add
assertions based on such unit-tested code.

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/ledger_pdf_generator_wx.cpp b/ledger_pdf_generator_wx.cpp
index f9ac78ec6..e6987f705 100644
--- a/ledger_pdf_generator_wx.cpp
+++ b/ledger_pdf_generator_wx.cpp
@@ -1610,60 +1610,65 @@ class page_with_tabular_report
 
         // The table may need several pages, loop over them.
         int const year_max = ledger.GetMaxLength();
-        for(int year = 0; year < year_max; )
+        for(int year = 0; year < year_max; ++year)
             {
-            int pos_y = render_or_measure_fixed_page_part
-                (table
-                ,writer
-                ,interpolate_html
-                ,oe_render
-                );
+            int pos_y = 0;
+            if(0 == year)
+                {
+                pos_y = render_or_measure_fixed_page_part
+                    (table
+                    ,writer
+                    ,interpolate_html
+                    ,oe_render
+                    );
+                }
 
-            for(;;)
+            for(std::size_t col = 0; col < columns.size(); ++col)
                 {
-                for(std::size_t col = 0; col < columns.size(); ++col)
-                    {
-                    std::string const variable_name = 
columns[col].variable_name;
-
-                    // Special hack for the dummy columns used in some reports,
-                    // whose value is always empty as it's used only as
-                    // separator.
-                    values[col] = variable_name.empty()
-                        ? std::string{}
-                        : interpolate_html.evaluate(variable_name, year)
-                        ;
-                    }
+                std::string const variable_name = columns[col].variable_name;
+
+                // Special hack for the dummy columns used in some reports,
+                // whose value is always empty as it's used only as
+                // separator.
+                values[col] = variable_name.empty()
+                    ? std::string{}
+                    : interpolate_html.evaluate(variable_name, year)
+                    ;
+                }
 
-                table.output_row(&pos_y, values.data());
+            table.output_row(&pos_y, values.data());
 
-                ++year;
-                if(year == year_max)
+            // Why is 'year' used above, but '1 + year' below?
+            // Is it that the first is in origin zero because that's
+            // natural in C++, but the second is in origin one because
+            // that's clearer with operations like '%'?
+            int next_year = 1 + year;
+            if(next_year == year_max)
+                {
+                // We will also leave the outer loop.
+                break;
+                }
+
+            if(next_year % rows_per_group == 0)
+                {
+                // We need a group break.
+                pos_y += row_height;
+
+                // And possibly a page break, which will be necessary if we
+                // don't have enough space for another group because we
+                // don't want to have page breaks in the middle of a group.
+                auto rows_in_next_group = rows_per_group;
+                if(year_max - next_year < rows_per_group)
                     {
-                    // We will also leave the outer loop.
-                    break;
+                    // The next group is the last one and will be incomplete.
+                    rows_in_next_group = year_max - next_year;
                     }
 
-                if(year % rows_per_group == 0)
+                if(pos_y > page_bottom - rows_in_next_group*row_height)
                     {
-                    // We need a group break.
-                    pos_y += row_height;
-
-                    // And possibly a page break, which will be necessary if we
-                    // don't have enough space for another group because we
-                    // don't want to have page breaks in the middle of a group.
-                    auto rows_in_next_group = rows_per_group;
-                    if(year_max - year < rows_per_group)
-                        {
-                        // The next group is the last one and will be 
incomplete.
-                        rows_in_next_group = year_max - year;
-                        }
-
-                    if(pos_y > page_bottom - rows_in_next_group*row_height)
-                        {
-                        next_page(writer);
-                        numbered_page::render(ledger, writer, 
interpolate_html);
-                        break;
-                        }
+                    next_page(writer);
+                    numbered_page::render(ledger, writer, interpolate_html);
+                    break;
                     }
                 }
             }
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------