Re: [lmi] [lmi-commits] master 43dcd1b 5/6: Make all table_column_info data members private and non-const

[Greg Chicares]

On 2018-08-24 00:07, Vadim Zeitlin wrote:
> On Thu, 23 Aug 2018 19:59:30 -0400 (EDT) Greg Chicares <address@hidden> wrote:
> 
> GC> branch: master
> GC> commit 43dcd1b788a27f55c67de39c07e62d6fce781ca2
> GC> Author: Gregory W. Chicares <address@hidden>
> GC> Commit: Gregory W. Chicares <address@hidden>
> GC> 
> GC>     Make all table_column_info data members private and non-const
> GC>     
> GC>     It is impossible to change members' values after construction, so 
> there
> GC>     is no longer any need for them to be const; and their constness was a
> GC>     problem because it made the class nonassignable.
> 
>  I don't really understand why the fields shouldn't be const if it's
> impossible to change them: IMHO this is exactly the reason to make them
> const.

These are all private members with public const accessors. No member
function (except for 'special member functions' in clause "[special]")
can change their values. Given that, what is the benefit of making
them (or most of them) const? The goal can't simply be to make them
somehow doubleplusconst; are you trying to prevent anyone from ever
adding a mutator that would change their values?

>  And are you sure that this class should be assignable? I think being
> movable should be enough.

Formerly, all were const and private except 'col_width_', which was
non-const and public. That troubled me: a table_column_info object
is 75% const, but I think a design is clearer and stronger when
constness is a binary attribute, either 100% or 0% (a chain is as
strong as its weakest link).

So I want col_width_ to have the same constness and accessibility
as its siblings, and you want its siblings to be const members.
Do you like the following patch, which I believe meets both those
criteria simultaneously?

(I had removed the member constness only so that I could assign
elements of the vector, or replace them with std::swap(); but
today I realized that the specialized std::vector::swap() does
not require that elements be Assignable.)

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/report_table.hpp b/report_table.hpp
index ccbae02a..0147223d 100644
--- a/report_table.hpp
+++ b/report_table.hpp
@@ -88,10 +88,10 @@ class LMI_SO table_column_info
     bool               is_clipped() const {return is_elastic();}
 
   private:
-    std::string   col_header_;
-    int           col_width_;
-    oenum_h_align alignment_;
-    bool          is_elastic_;
+    std::string   const col_header_;
+    int           const col_width_;
+    oenum_h_align const alignment_;
+    bool          const is_elastic_;
 };
 
 std::vector<int> LMI_SO apportion(std::vector<int> const& votes, int seats);
diff --git a/wx_table_generator.cpp b/wx_table_generator.cpp
index 35ca7974..be442273 100644
--- a/wx_table_generator.cpp
+++ b/wx_table_generator.cpp
@@ -66,15 +66,18 @@ wx_table_generator::wx_table_generator
         ,2 * one_em_
         ,one_puncsp
         );
+
+    std::vector<table_column_info> resized_columns;
     for(int j = 0; j < lmi::ssize(all_columns()); ++j)
         {
-        all_columns_[j] = table_column_info
+        resized_columns.emplace_back
             (all_columns_[j].col_header()
             ,w           [j]
             ,all_columns_[j].alignment()
             ,all_columns_[j].is_elastic() ? oe_elastic : oe_inelastic
             );
         }
+    all_columns_.swap(resized_columns);
 
     // Set a pen with zero width to make grid lines thin,
     // and round cap style so that they combine seamlessly.
@@ -114,15 +117,18 @@ wx_table_generator::wx_table_generator
         ,2 * one_em_
         ,one_puncsp
         );
+
+    std::vector<table_column_info> resized_columns;
     for(int j = 0; j < lmi::ssize(all_columns()); ++j)
         {
-        all_columns_[j] = table_column_info
+        resized_columns.emplace_back
             (all_columns_[j].col_header()
             ,w           [j]
             ,all_columns_[j].alignment()
             ,all_columns_[j].is_elastic() ? oe_elastic : oe_inelastic
             );
         }
+    all_columns_.swap(resized_columns);
 
     dc_.SetPen(illustration_rule_color);
 }
@@ -435,10 +441,7 @@ LMI_ASSERT(std::size_t(h / lh) == 1u + 
count_newlines(z.header));
             break;
         }
 
-    all_columns_.push_back
-        (table_column_info
-            (z.header, width, z.alignment, z.elasticity)
-        );
+    all_columns_.emplace_back(z.header, width, z.alignment, z.elasticity);
 }
 
 void wx_table_generator::do_output_single_row
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------