Re: [lmi] Delegating ctor uncalled for?

[Vadim Zeitlin]

On Tue, 7 Feb 2017 04:58:33 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-02-04 02:16, Vadim Zeitlin wrote:
GC> [...]
GC> >  OTOH the current code still doesn't seem to be ideal to me because:
GC> > 
GC> > 1. It definitely looks like it ought to be possible to reuse the code
GC> >    of the 3 ctors taking vectors in some way.
GC> 
GC> Please comment on this attempt. No comment is too trivial.
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/input_sequence.cpp b/input_sequence.cpp
GC> index 101fa26..2433767 100644
GC> --- a/input_sequence.cpp
GC> +++ b/input_sequence.cpp
GC> @@ -38,6 +38,7 @@
GC>  #include <ostream>
GC>  #include <sstream>
GC>  #include <stdexcept>
GC> +#include <type_traits>

 Spoiler: I hope to convince you that this header is not really needed.

GC> +// See commentary in header.
GC> +void set_value(ValueInterval& v, double d)

 To get this out of the way, I'm not sure what's the point of having this
global function -- and the code using it. If it's just to demonstrate that
it's less readable and more inconvenient than using a method, then I fully
agree with it. If it's something else, then could you please explain what
is it?

GC> -InputSequence::InputSequence(std::vector<std::string> const& v)
GC> +template<typename T>
GC> +InputSequence::InputSequence(std::vector<T> const& v)
GC>      :years_to_maturity_(v.size())
GC>  {
GC> +    bool const T_is_double = std::is_same<T,double     >::value;
GC> +    bool const T_is_string = std::is_same<T,std::string>::value;
GC> +    static_assert(T_is_string || T_is_double, "");

 As always with preconditions, it's nice to check them in the function, but
it's even nicer to make it impossible to call the function with the wrong
parameters. In this particular case, I want to suggest keeping two public
ctors taking std::vector<double> and <string> respectively but delegate
both of them to this new _private_ template ctor. IMHO this would result in
the best "programmer experience": it will be clear from just looking at the
header that an InputSequence can be created only from a vector of numbers
or a vector of strings and not an arbitrary vector of anything else and
attempts to do the latter wouldn't even compile which is even better than
not linking, as it would currently happen.

 In practice there is a slight problem with having a private template ctor
because the public ones can't delegate to it, doing it would just result in
an infinite recursion as they would delegate to themselves. There are two
solutions for this:

1. Keep using the template ctor but give it a dummy argument, e.g. add some
   (also private) "class call_template_ctor_tag" and then write delegating
   ctors as

    explicit InputSequence(std::vector<double> const& v)
        :InputSequence(v, call_template_ctor_tag{})
        {
        }

   This is a bit verbose, but readable and expresses the purpose of the
   code quite well IMHO.

2. Much simpler: just forget about delegating ctors and use a simple
   initialize(std::vector<T> const&) function which would be called
   from both public ctors. The only "cost" of this approach is that we'll
   have to abandon the use of member initializer list for
   years_to_maturity_ and use assignment inside initialize() function
   instead -- but this is hardly a big price to pay, so I'd just do it like
   this (but keep in mind (1) in case we have any similar situations in
   which we have some more expensive member initialization to perform).


GC>      ValueInterval dummy;
GC> -    dummy.value_is_keyword = true;
GC> +    dummy.value_is_keyword = T_is_string;

 Here I want to argue in favour of setting value_is_keyword in set_value()
itself, please see below.

GC> +// Can't do this because the "if" and "else" parts can't both compile:
GC> +//  if(T_is_string) intervals_.back().value_keyword = current_value;
GC> +//  else            intervals_.back().value_number  = current_value;

 Yes, this would require C++17 constexpr if (confusing name for the "if
constexpr" language construct, I've never even used it yet, but I'm already
sure I'm going to misplace this "constexpr" in at least 50% of cases).

GC> +// I couldn't figure out any simpler syntax that omits the arguments...

 I'm not sure what do you mean by "omitting the arguments", i.e. what would
you expect to happen if the argument could be omitted?

GC> +template InputSequence::InputSequence(std::vector<double>      const&);
GC> +template InputSequence::InputSequence(std::vector<std::string> const&);

 Also, if you follow my idea of using private template initialize(), we
could get rid of the need to use these explicit instantiations completely:
we'd just need to define the public ctors using initialize<double>() and
initialize<std::string>() in input_sequence.cpp, to force instantiating
them implicitly when compiling this file. If we wanted to keep public ctors
inline, then we'd still need these explicit instantiations, but at least we
could (and should, IMO) have a comment explaining that they're needed
because they're used from these two public ctors -- and that nobody else
can use them anyhow.


 BTW, this would, again, be irrelevant anyhow if you follow my idea of
using a private template ctor or a function, but I thought to use another
C++ feature I had never used before, even though it appeared in C++11,
called "explicit instantiation declarations" a.k.a. "extern templates". I
believed I could prevent implicit instantiation from compiling in this way,
but it looks like I misunderstood how this worked because the code using
InputSequence ctor from std::vector<bool> still compiled just fine -- even
though it failed to link, of course. Now I wonder what exactly extern
templates are useful for...


GC> +// According to this style guide:
GC> +//   https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

 I am suspicious of Google C++ style guides. This one is not as bad as the
old one but it still forbids the use of exceptions which is an extremely
questionable decision in C++. I'd rather recommend referring to the C++
Core Guidelines, which, of course, shouldn't be taken as a gospel neither
(and, inevitably, I do disagree with at least a few things in them), but
seem much more reasonable globally. In this particular case the relevant
guideline is

        C.2: Use class if the class has an invariant; use struct if the
        data members can vary independently

(see http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-struct)

GC> +// "structs should be used for passive objects that carry data, and may
GC> +// have associated constants, but lack any functionality other than
GC> +// access/setting the data members. The accessing/setting of fields is
GC> +// done by directly accessing the fields rather than through method
GC> +// invocations. Methods should not provide behavior but should only be
GC> +// used to set up the data members, e.g., constructor, destructor,
GC> +// Initialize(), Reset(), Validate()."

 I think the above is a much better formulation of the same idea and,
according to C.2, ValueInterval must be a class because it does have an
invariant: "value_keyword is valid iff value_is_keyword is true". And based
on this I strongly believe that set_value() must modify value_is_keyword in
addition to changing the value. In fact, I'd argue for using a
variant<double, std::string> here instead of these 3 fields, to make things
even more type-safe, but considering our reluctance to start using Boost
libraries and impossibility to start using C++17 just yet, this is a
non-starter, of course.

GC> +// OTOH, classes shouldn't have public nonconst data members.

 I disagree with this. If it doesn't make sense to abstract it and if there
are no invariants involving this member, there is no good reason to not
provide public access to it.

 So, combining both points, I would make ValueInterval a class, but keep
all of its members except for value_xxx public, while providing get_value()
accessors, checking that the correct field is being accessed, for
value_{number,keyword}. Of course, this is not directly related to these
changes, so it could be done before or after them (or not at all).


GC> -    InputSequence(std::vector<double> const&);
GC> -    InputSequence(std::vector<std::string> const&);
GC> +    template<typename T>
GC> +    InputSequence(std::vector<T> const& v);

 This is minor but, template or not, this ctor should be explicit IMO.


 To summarize, there are 2 important points I tried to make:

1. Don't expose a public template ctor, this provides a too broad
   interface that InputSequence can't implement. Instead, provide the
   same 2 public ctors and implement them in a single template function.

2. Don't allow manipulating value_is_keyword independently but set it when
   calling set_value(). Later it would be nice to make ValueInterval a
   class and stop providing direct access to value_{number,keyword} to
   ensure that the invariant can't be broken.


 Thanks for reading all this!
VZ