Re: [lmi] Delegating ctor uncalled for?

[Greg Chicares]

On 2017-02-07 14:37, Vadim Zeitlin wrote:
> 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.

I'm going to defer that question for the moment so that I can commit all
of the other changes.

> 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.

Yes, the only intention was to demonstrate exactly that.

> 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.

Yes, I agree so far, except that I think it's even nicer to retain those
precondition checks in the member function template, at least for now.
Let me make the commits I have in mind; then, if you like, we can return
to this question.

>  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.

Yes, BTDT, and it was no fun.

> There are two
> solutions for this:
> 
> 1. Keep using the template ctor but give it a dummy argument
[...]
>    This is a bit verbose, but readable and expresses the purpose of the
>    code quite well IMHO.

I agree that verbosity, readability, and expressiveness are the criteria,
but I would score them differently. I'd see this as unnecessarily verbose
and needlessly obscure. I'd consider it imperative to add a comment to
explain such a weird trick, and it is the comment rather than the trick
that would express the intention, to me at least.

> 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.

That's the way (uh huh uh huh).

>    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).

That's not even a cost at all. The real ctors that "delegate" to this
member function template retain their one-element initializer lists;
that's absolutely an appropriate thing for ctors to do, and I think it
is clearer for them to do that than to "delegate" it. Similarly, I'll
keep the call to realize_vector() (which, BTW, I'll rename) in each of
these ctors. Result: both ctors set years_to_maturity_ in an initializer
list, and both establish the universal postconditions for all ctors by
calling realize_vector()--i.e., all three ctors establish postconditions
in exactly the same explicit way. You could say that it "wastes" two
lines to do this for each, but I'd argue that it increases clarity. As
above, wait and see what I'll be committing soon, and we can discuss
this again afterwards if you like.

> 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.

Deferred.

> 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).

AFAICT:
  1998: C++ version one.
    2003: One point one.
  2011: C++ version two.
    2014, 2017: two point one, two point two.
The most important thing is to upgrade lmi to version two.

Although I really wish C++11's static_assert didn't require that extra
string argument.

> 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?

What I had in mind was
  tn_range_types.cpp:template class trammel_base<calendar_date>;
where I could just write
  "template", (name of class), <type>, (semicolon)
but that only works for class templates. No such syntax as
  "template", (name of function), <type>, (semicolon)
could possibly work because functions can be overloaded.

And that's why I had to write it this way:
  template InputSequence::InputSequence(std::vector<std::string> const&);

> 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

Yes, absolutely.

>  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...

I actually thought of that when I was writing the patch we're discussing
here, so I looked it up...and came away with the same question. I just
don't see any reason why I'd want to use them.

Long ago I learned how to manage explicit-only instantiation, probably
even before C++98. It was necessary in those days to avoid code bloat.
Perhaps today's smart linkers obviate that formerly imperative motivation,
but there's a very good reason to keep using the old idiom: it lets us
write the implementation of our member function template in the '.cpp'
file. In the first draft of what became the patch I posted, I wrote that
implementation in the header, just because I knew that was guaranteed to
work; but I quickly decided that if I couldn't move it back into the
'.cpp' file, then I'd just abandon the whole idea of using a template
at all. I want to be able to understand the implementation by reading
only the '.cpp' file; I really dislike flipping back and forth between
it and the header when the implementation sprawls across both.

So I think I already have a compellingly superior solution for whatever
problem 'extern template' might have been meant to solve. As always,
maybe I'm missing something.

> 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

I prefaced my patch with:

> GC> Please comment on this attempt. No comment is too trivial.
                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^

for the exact purpose of inviting such comments.

> but, template or not, this ctor should be explicit IMO.

Correct.

>  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.

Agreed.

> 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.

Deferred for later discussion.

>  Thanks for reading all this!

Thank you for writing it.