Re: [lmi] Delegating ctor uncalled for?

[Vadim Zeitlin]

On Sat, 4 Feb 2017 12:58:39 +0000 Greg Chicares <address@hidden> wrote:

GC> If you have any specific questions, I'll try to answer them.

 The basic question I have is why do we have these 4 different ctors and
when each of them is supposed to be used. The first one is relatively
clear, it's used to parse the "input expression" string, but the other ones
are not obvious to me. And even the first one is not totally obvious
because I'm not sure what exactly is the separation line between it and
SequenceParser. E.g. why not have SequenceParser::sequence() as a factory
function for making InputSequences?


GC> I think you're looking for the "canonical representation", alluded to in
GC> one of the remaining "TODO" comments, which has not yet been defined.

 I was indeed thinking of a canonical representation, but at C++ types,
not necessarily string/textual, level. Of course, if we had such a
representation at C++ level it would be trivial to convert it to a string.

GC> > 2. The "numerous arguments" ctor is IMO unwieldy and difficult to both
GC> >    use and read. I would strongly consider using named arguments idiom
GC> >    in order to avoid having a function with 9 parameters which is about
GC> >    twice greater than optimal.
GC> 
GC> I'm not sure whether you mean boost::parameter or this:
GC>   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4172.htm

 Sorry, I should have been more clear: I meant the approximation of the
latter available in the current C++. It is described (not very well) at
https://en.wikibooks.org/wiki/More_C++_Idioms/Named_Parameter and used in
wxWidgets with wxSizerFlags or wxFontInfo, for example.

GC> But there are only a few combinations of defaulted parameters that
GC> actually make sense.

 This is actually a big red flag for me, IMO the class shouldn't provide a
ctor allowing to pass it incompatible values of the arguments. With the
named parameters idiom it would be simple enough to ensure that this can't
happen, e.g. by having two separate methods such as allowed_keywords() and
allowed_keywords_with_default() (which could specify the default as an
index in the provided keywords array).

GC> The number of arguments per se doesn't bother me because I know
GC> that the five 'int' arguments are a unitary, interdependent set
GC> extracted from class Input, so notionally the arguments are
GC>   std::string const& expression
GC>   Input const&
GC> and the three optional ones.

 It's still simple enough to make a mistake with the order of these 5
arguments. Maybe I just make too many mistakes, but I really appreciate
APIs which prevent me from making them, so I'd strongly prefer to be able
to write

        InputSequence sequence
                (expression
                ,InputParameters()
                        .years_to_maturity(y2m)
                        .issue_age(ia)
                        .retirement_age(ra)
                        .inforce_duration(id)
                        .effective_year(ey)
                );

GC> If OTOH the motivation is to make a change like this easier:

 To be totally clear, my motivations are, in order:

1. Make it impossible to accidentally make a mistake in the function call.
2. Make it easier to read the calls to the code.
3. Make it simpler to add more input parameters in the future.

GC>   commit 0482a6a9a8e78696b8bc1a2a60ccfcb0e140c329
GC>   Remove default keyword from SequenceParser; reorder it in InputSequence
GC> 
GC> then yes, it would have had that effect, but that's the first time
GC> since this class was introduced (probably in 2002) that the argument
GC> list has changed,

 Yes, I understand that this doesn't happen very often, which is why (3) is
the last item in my list above. Nevertheless, I must say that it's very
nice to be able to add new parameters trivially easy, without worrying
about breaking the existing code, when it does happen.

GC> and I would very much rather make such simple
GC> changes than introduce yet another dependency on yet another boost
GC> library.

 FWIW I definitely didn't mean to propose to start using boost::parameters,
I don't really like it as it seems too complicated and "magic". It's
impressive that such a library could be written, but I'd probably only use
it if I had hundreds of functions requiring its use, not just a few.


GC> Yes, and furthermore you've encouraged me to
GC>   :setlocal spell spelllang=en_us

 Ah, I see you've discovered my secret to perfect spelling (if only Vim
could also check my phrases for their meaning too...).

 Regards,
VZ