Re: [lmi] [lmi-commits] master 9b3d0f1: Write explicitly-defaulted [cd]tors inline, in class defn

[Greg Chicares]

On 2017-03-05 20:55, Vadim Zeitlin wrote:
> On Sun, 5 Mar 2017 20:43:31 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2017-03-05 19:54, Vadim Zeitlin wrote:
> GC> > On Sun,  5 Mar 2017 14:28:23 -0500 (EST) Greg Chicares <address@hidden> 
> wrote:
[...]
> GC> > GC> -    double ScalarIMF_;
> GC> > GC> -    std::string ShortName_;
> GC> > GC> -    std::string LongName_;
> GC> > GC> -    std::string gloss_;
> GC> > GC> +    double      ScalarIMF_ = 0.0;
> GC> > GC> +    std::string ShortName_ = std::string();
> GC> > GC> +    std::string LongName_  = std::string();
> GC> > GC> +    std::string gloss_     = std::string();
> GC> > GC>  };
> GC> > 
> GC> >  The last 3 new lines above also seem unnecessarily verbose to me, IMHO
> GC> > this is what default ctors are for and we could just omit the
> GC> > initialization entirely, but if we wanted to keep it for explicitness 
> sake,
> GC> > I'd use "std::string ShortName_{};" etc here.
> GC> 
> GC> In this and the other examples you gave, I thought I was following
> GC> the rule we agreed on, e.g., here:
> GC> 
> GC> void input_sequence_test::check
> GC>     (char const*                     file
> GC> ...
> GC>     ,char const*                     m = ""
> GC>     ,std::vector<std::string> const& k = std::vector<std::string>()
> GC>     ,std::vector<std::string> const& c = std::vector<std::string>()
> GC>     ,bool                            o = false
> GC>     ,std::string const&              w = std::string()
> GC> 
> GC> But even if I remember that correctly, there's no reason why our
> GC> thinking shouldn't evolve.
> 
>  I thought the rule applied to the function arguments, but I didn't realize
> it would be applied to the in-class member initialization too.

If only uniform initialization were more...uniform.

This is a cognitive obstacle for me. I moved a ctor-initializer, i.e. a
mem-initializer list, to a set of brace-or-equal-initializers:

-FundInfo::FundInfo()
-    :ScalarIMF_(0.0)
-    ,ShortName_("")
-    ,LongName_ ("")
-    ,gloss_    ("")

-    double ScalarIMF_;
-    std::string ShortName_;
-    std::string LongName_;
-    std::string gloss_;
+    double      ScalarIMF_ = 0.0;
+    std::string ShortName_ = std::string();
+    std::string LongName_  = std::string();
+    std::string gloss_     = std::string();

Under the C++98 rules, the ctor-initializer requires
  name (value) [,]
and we could have written it as simple statements in the ctor body:
  name = value ;
(except that's less good, of course). This is not uniform, but
there's only one way to do each of these things, so we learned soon
enough because the compiler enforce these requirements.

But now I just want to migrate the ctor-initializers into C++11
brace-or-init^W equal-or-brace^W equal-or-brace-init^W
brace-or-equal-initializers, so I substitute an equals sign for the
parentheses, change the empty string from "" to std::string() (which
it always should have been for efficiency...except for verbosity),
and...and...I get confused and apply the rule for an initializer-clause
in a function's parameter-declaration-list.

Let's hope this gets easier with practice, but for now...

> I really
> need to at least create the style guidelines document I keep speaking about
> if only in order to add the new rules to it.

...that would be most helpful for me.

>  Anyhow, in the light of these examples, what do you now think the rule
> should be for the member initialization?

Let me start by saying what the rule perhaps shouldn't be.

Mechanically, I visual-line-yanked this:

-    :ScalarIMF_(0.0)
-    ,ShortName_("")

and pasted it near this:

    double ScalarIMF_;
    std::string ShortName_;
---------
    :ScalarIMF_(0.0)
    ,ShortName_("")

and then merged them into

-    double ScalarIMF_;
+    double      ScalarIMF_ = 0.0;

which was easy enough, and then

-    std::string ShortName_;
+    std::string ShortName_ = std::string();

which was not so easy. The reason why I laboriously recite this again
is to establish a framework for cases that are less simple:

-    :CachedDate_(jdn_t(0))
-    mutable calendar_date CachedDate_;
+    mutable calendar_date CachedDate_ = jdn_t(0);
[oops, compiler error]
+    mutable calendar_date CachedDate_ = calendar_date(jdn_t(0));

or:

-    :decimals_ (0              )
-    ,style_    (r_indeterminate)
-    ,gloss_    (""             )

-    int                decimals_;
-    mce_rounding_style style_   ;
-    std::string        gloss_   ;
+    int                decimals_ = 0;
+    mce_rounding_style style_    = mce_rounding_style(r_indeterminate);
+    std::string        gloss_    = std::string();

That's bletchery.

And it becomes worse IMO if we add a rule to elide the initializer for
types like std::string that already happen to have the default initializer
that we really want. IOW, removing three initializers...

-    :decimals_ (0              )
-    ,style_    (r_indeterminate)
-    ,gloss_    (""             )

...and adding two...

-    int                decimals_;
-    mce_rounding_style style_   ;
+    int                decimals_ = 0;
+    mce_rounding_style style_    = mce_rounding_style(r_indeterminate);

...while leaving the third untouched...

     std::string        gloss_;

...because, if you devote more thought to it than I can spare, it already
does the right thing: this is just too much. Sure, in isolation, I can
reason that out; but this was part of a changeset of about 75 files, and
reviewing that whole 'git diff' before committing it was already hard
enough without worrying about special cases. ("Wait, I removed three
inits, but added two...so let's delve deeper into this change, and, okay,
one of the three was a std::string, and so...").

Besides, afterwards I'd like to be able to look at the data members of
a class and see all of them initialized, all in a visually similar way,
so that I can see whether I missed any.

I guess that's my long-winded way of saying this:

> Personally, I like using the
> simple "member_ = xxx" for primitive types such as int or bool, because it
> seems more readable to me, as the values are clearly separated from the
> fields, especially with the vertical alignment convention used in lmi, but
> I also like to use "member_{xxx}" for the non-primitive types to avoid
> repeating the type name, especially in the special case of "member_{}", if
> we decide to explicitly default-initialize all members. The latter seems to
> outweigh the former, so I think we should just standardize on using uniform
> initialization here (and also in the member initialization lists because
> they are very similar).
>  
>  What do you think?

Yes, the latter...but with all the benefits above:

 - "values are clearly separated from the fields": We can use "the vertical
   alignment convention used in lmi" for separation. (It's the quirk I'm
   proudest of. I'm sure that two-dimensional information should be laid out
   in two dimensions, whether or not I can convince anyone else.)

 - "avoid repeating the type name": Absolutely.

 - "if we decide to explicitly default-initialize all members": Before meals,
   which fingers on which hands do you wash? After meals, which teeth do you
   brush? Maybe it's just my personality, but I have an intense desire for
   regularity.

I thought of combining the ideas: an explicit '=', and '{}' (at least when
it's needed...and I discovered something interesting (to me):

 - this compiles:
    int                decimals_ {0};
    mce_rounding_style style_    {r_indeterminate};
    std::string        gloss_    {};

 - but this does not:
    int                decimals_ = {0};
    mce_rounding_style style_    = {r_indeterminate};
    std::string        gloss_    = {};

    error: converting to 'mce_rounding_style {aka mc_enum<rounding_style>}'
    from initializer list would use explicit constructor
    'mc_enum<T>::mc_enum(T) [with T = rounding_style]'
      mce_rounding_style style_    = {r_indeterminate};
                                                     ^
It makes sense--I just didn't know about it.

Anyway, in conclusion, if you really want to write "int foo = 0;", I'd be
happy enough with this:

    int                decimals_ =  0;
    mce_rounding_style style_      {r_indeterminate};
    std::string        gloss_      {};

because, thanks to two-dimensional alignment, I can see that every data
member has an initializer--even with no '=' to serve as a delimiter.
But I don't suspect you'll like that at all, so I think this is the
One True Way:

    int                decimals_ {0};
    mce_rounding_style style_    {r_indeterminate};
    std::string        gloss_    {};

I will go back and regularize 9b3d0f1134744e6fe7e4bba8ffa5e8a655f7648b,
but first let me disclose my full plan. The commit immediately before
that one changed a hundred files in a way that's simple as dirt and
disposed of most of my planned changeset without controversy. Then
9b3d0f1 disposed of the files that seemed easy (but led to this most
illuminating discussion). However, I still have twenty-one files to go,
which are the thorniest; I've already tested all changes, including
those, so I'm going to deal with the last twenty-one first.