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 14:38, Vadim Zeitlin wrote:
> On Fri, 24 Aug 2018 14:30:50 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2018-08-24 00:07, Vadim Zeitlin wrote:
> GC> > On Thu, 23 Aug 2018 19:59:30 -0400 (EDT) Greg Chicares <address@hidden> 
> wrote:
> GC> > 
> GC> > GC> branch: master
> GC> > GC> commit 43dcd1b788a27f55c67de39c07e62d6fce781ca2
> GC> > GC> Author: Gregory W. Chicares <address@hidden>
> GC> > GC> Commit: Gregory W. Chicares <address@hidden>
> GC> > GC> 
> GC> > GC>     Make all table_column_info data members private and non-const
> GC> > GC>     
> GC> > GC>     It is impossible to change members' values after construction, 
> so there
> GC> > GC>     is no longer any need for them to be const; and their constness 
> was a
> GC> > GC>     problem because it made the class nonassignable.
> GC> > 
> GC> >  I don't really understand why the fields shouldn't be const if it's
> GC> > impossible to change them: IMHO this is exactly the reason to make them
> GC> > const.
> GC> 
> GC> These are all private members with public const accessors.
> 
>  If you can forgive my nitpicking, I don't really like this on its own
> neither: what's the benefit of encapsulation if we provide trivial
> accessors for the fields anyhow? I think just exposing the members as they
> are is good enough when they're const and is simpler and, in some sense,
> more honest than creating an illusion of encapsulation when there is none.

Upstream clients have, variously,
  struct column_definition
  struct illustration_table_column
and here we have
  table_column_info
all of which embody much the same information. There are so many complex
interactions across so much code that it's not possible thoroughly to
redesign any part in isolation, so gradualism is the strategy. Bearing
in mind the Great Helmsman's injunction to create great disorder under
heaven in order to achieve great order under heaven, I'm viewing it as
a simulated-annealing problem. Some members in the various structs and
classes are const, others are not, and a couple are even, noxiously,
'mutable'; some are strings, while others are char const*; some are
enumerative where corresponding members are boolean. As we raise the
"temperature", flaws will relax away; as "temperature" then reduces,
an ideal design will begin to crystallize.

For you, the natural state of a bunch-of-data may be a struct with const
members; for me, it's a class with private members and const accessors.
I might not mind if such a class collapses into a struct eventually
because that's the ground state it annealed into. But until then, let
a hundred flowers bloom; let a hundred schools of though contend. I
don't know whether other, initially-unanticipated member functions
will arise, and I prefer the class because it's more amenable to
that. The day when all that becomes clear is a long way off.

> GC> Given that, what is the benefit of making them (or most of them) const?
> 
>  Sorry if it's a trick question, but my naive answer is that, of course,
> making them "const" ensures they're not mutated in the class code itself
> and not only from outside it. And if they really don't have to be changed,
> this can only be a good thing.

I suspect you want members to be const because you want them to be
contained in a struct, while I see it as pointless to make them
const because I want them to be private members in a class. I was
just wondering whether there was a substantive, concrete, practical
difference. The only one I see is that the class with non-const
members is Assignable. But emplacement means Assignability is not
really a requirement.

> GC> So I want col_width_ to have the same constness and accessibility
> GC> as its siblings, and you want its siblings to be const members.
> GC> Do you like the following patch, which I believe meets both those
> GC> criteria simultaneously?
> 
>  Yes, thank you, I do agree that this is better.

I'm committing that now, not to rule out further discussion, but
only to pop an agreed-upon incremental improvement off my stack
as I prepare to put work aside briefly to address a family
obligation.

> But I don't understand why
> do we need to use resized_columns and swap(), what exactly prevents us from
> emplacing the objects directly into all_columns_?

Glad you asked. I looked for std::vector::emplace() but didn't find
any such member, so I figured maybe we'll get it in a later standard.
Looking again more carefully, I now see that it does exist. However,
it seems to have the same semantics as insert(), whereas I was
hoping for an equivalent of "operator[]=" with move semantics.
I see it as risky to replace all the members of a vector by inserting
a new version of each and then necessarily removing the old: there
are just too many ways to make a mistake when coding that.

Actually, my next idea is to abstract
  for(...) enroll_column()
  set_column_widths()
  emplace new width
into a free function, hoping to call it in the ctor-initializer:
  wx_table_generator::wx_table_generator(...)
    :all_columns_ {magical_new_function(vc)}
in order to enable this change:
-    std::vector<table_column_info> all_columns_;
+    std::vector<table_column_info> const all_columns_;
because I think that 'const' would be most desirable. And I'd want
  vector<something> magic_function(vector<something> const&)
for that purpose, so I'd have to construct a new object anyway.

Of course, setting max_header_lines_ in enroll_column() is very
convenient today, but constitutes an obstacle to writing this
magical_new_function(). Should table_column_info gain a new
header_lines_ member? I'd resist adding that. Maybe the
max_header_lines_ calculation is so cheap that it can be done
inline in the ctor, or in a new wx_table_generator member
function...yes, if we revert to using count_newlines() and
forsake the tempting idea to let GetMultiLineTextExtent()
handle that instead--good thing I kept that design fluid.

Anyway, I've gone into more detail than usual because I don't
have time to say less, and now I really must sign off.