Re: [lmi] [lmi-commits] master c59d263 4/5: Replace C-style array with vector

[Greg Chicares]

On 2018-04-25 17:26, Vadim Zeitlin wrote:
> On Wed, 25 Apr 2018 10:53:48 -0400 (EDT) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit c59d2632f6019ea31607fa2cecba69b707fb8ed9
> GC> Author: Gregory W. Chicares <address@hidden>
> GC> Commit: Gregory W. Chicares <address@hidden>
> GC> 
> GC>     Replace C-style array with vector
> GC> ---
> GC>  group_quote_pdf_gen_wx.cpp | 5 +++--
> GC>  1 file changed, 3 insertions(+), 2 deletions(-)
> GC> 
> GC> diff --git a/group_quote_pdf_gen_wx.cpp b/group_quote_pdf_gen_wx.cpp
> GC> index f302e36..1076110 100644
> GC> --- a/group_quote_pdf_gen_wx.cpp
> GC> +++ b/group_quote_pdf_gen_wx.cpp
> GC> @@ -346,7 +346,8 @@ class group_quote_pdf_generator_wx
> GC>  
> GC>      struct row_data
> GC>          {
> GC> -        std::string output_values[e_col_max];
> GC> +        row_data() {output_values.resize(e_col_max);}
> GC> +        std::vector<std::string> output_values;
> GC>          };
> GC>      std::vector<row_data> rows_;
> 
>  Why not just use
> 
>       struct row_data
>           {
>           std::vector<std::string> output_values{e_col_max};
>           };

I tried writing exactly that, but with "()" instead of "{}", which makes
it some kind of unholy function declaration. Then I looked at the "[]"
in the original and, realizing that square brackets would certainly be
wrong, concluded that parentheses must be right. "{}" is like the passé
simple: I can read it just fine, but I never quite learned to write it.
But keep reminding me, because I shouldn't keep this as a blind spot.

Anyway...committed. Thanks.

> ? Although, to be honest, I'd really prefer to used std::array here because
> the main point of using an array and not a vector here originally was to
> show that this array is _not_ resizable (this code predates the switch to
> C++11 in lmi, so std::array wasn't an option back then, otherwise I'd have
> used it). Using a vector doesn't only incur the unnecessary (but, at least
> in isolation, unnoticeable) overhead of the heap allocation but makes the
> code less clear IMO, as you'd expect this vector to be appended to,
> somewhere, whereas we never do this.

True; but OTOH, IIRC, every other argument to do_output_values() and
output_row() was of type std::vector, and I favor consistency. In
'ledger_pdf_generator_wx.cpp', why not change this line, e.g.:
    std::vector<std::string> output_values(columns.size());
to use an array as well? One container type should be used in all
similar situations.

I have little appetite for reopening this refactoring exercise again,
but if I had, this pair of lines from two similar files troubles me
more than the possibility of resizing a vector (which I now wouldn't
dream of doing in this code):

            table.output_row(&pos_y,   output_values);
        table_gen.output_row(&pos_y, i.output_values);

and I had half a mind to s/\<table\>/table_gen/ because the latter
name is greppable.

But what really bothers me is that I only half rewrote
wx_table_generator::output_header(). I never would have imagined
slicing N contiguous elements from a vector of M*N elements as
  &vec.at(k); // for k in {1..N-1}
because I've never seen '&' written before a std::vector variable;
and I really wanted to make the data a matrix, but C++ containers
are strangely limited to one dimension, so this can't be written
as naturally as we would have done sixty years ago in APL or
even FORTRAN. (OTOH, C++ does have lower-case letters.)

>  Considering the following commit, I think the chances of the change I
> propose are slim, but if the vector remains, I'd at least add a comment
> explaining that the size of this vector never changes and we use it here
> instead of an array just because... I don't actually know why, but you
> should be able to fill in the dots.

I had already begun these changes before I fully realized that this
entire implementation is premised on a fundamental decision to hold
data in containers of immutable size, and that that's not only
(1) intentional, but (2) actually not unsound, and (3) even reasonable.
It still feels utterly alien to me, but so do some of AlphaGo's moves.