Re: [lmi] Delegating ctor uncalled for?

[Greg Chicares]

On 2017-02-04 15:23, Vadim Zeitlin wrote:
> 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.

Now there are three ctors, and only one of them is public.

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

What do you think of the patch below[0]? The question I'm struggling with
is whether it's worth an extra fifty lines of code just to say "don't use
class SequenceParser directly"...though it would require only half as many
extra lines if we use the following idea...

> 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.
[...]
> [...] 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)
>               );

Okay, then InputParameters (we'd give it a more specific name) would be
an auxiliary struct containing approximately those arguments (and maybe
a couple more). I'm starting to like that idea because it would reduce
the size of the patch below, and also of the size of some recent changes
to 'input_sequence_entry.cpp'.

---------

[0] "the patch below": This makes class SequenceParser unusable except
by a friend factory function. The copy ctor cannot be defaulted because
of the std::[io]stringstream members; and perhaps it should not be
defaulted anyway, because only two members are suitable for client use.

$git diff --stat                     
 input_sequence.cpp        |  4 ++--
 input_sequence_parser.cpp | 39 +++++++++++++++++++++++++++++++++++++--
 input_sequence_parser.hpp | 45 ++++++++++++++++++++++++++++++++-------------
 3 files changed, 71 insertions(+), 17 deletions(-)

54 lines are added, but 28 of those lines consist of four new full
recitations of a list of seven common parameters.

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/input_sequence.cpp b/input_sequence.cpp
index fd20f10..9088199 100644
--- a/input_sequence.cpp
+++ b/input_sequence.cpp
@@ -94,7 +94,7 @@ InputSequence::InputSequence
         || a_keywords_only && contains(a_allowed_keywords, a_default_keyword)
         );
 
-    SequenceParser parser
+    SequenceParser const& parser = ParseSequence
         (input_expression
         ,a_years_to_maturity
         ,a_issue_age
@@ -105,7 +105,7 @@ InputSequence::InputSequence
         ,a_keywords_only
         );
 
-    std::string const parser_diagnostics = parser.diagnostics();
+    std::string const parser_diagnostics = parser.diagnostic_messages();
     if(!parser_diagnostics.empty())
         {
         throw std::runtime_error(parser_diagnostics);
diff --git a/input_sequence_parser.cpp b/input_sequence_parser.cpp
index 74cdd10..183980f 100644
--- a/input_sequence_parser.cpp
+++ b/input_sequence_parser.cpp
@@ -51,13 +51,25 @@ SequenceParser::SequenceParser
     ,keywords_only_                 (a_keywords_only)
 {
     sequence();
+    diagnostic_messages_ = diagnostics_.str();
+}
+
+/// Copy ctor: copies only two data members.
+///
+/// The public interface consists only of the dtor and accessors for
+/// the two copied data members.
+
+SequenceParser::SequenceParser(SequenceParser const& z)
+{
+    diagnostic_messages_ = z.diagnostic_messages_;
+    intervals_           = z.intervals_          ;
 }
 
 SequenceParser::~SequenceParser() = default;
 
-std::string SequenceParser::diagnostics() const
+std::string SequenceParser::diagnostic_messages() const
 {
-    return diagnostics_.str();
+    return diagnostic_messages_;
 }
 
 std::vector<ValueInterval> const& SequenceParser::intervals() const
@@ -655,6 +667,29 @@ void SequenceParser::mark_diagnostic_context()
         ;
 }
 
+SequenceParser ParseSequence
+    (std::string const&              input_expression
+    ,int                             a_years_to_maturity
+    ,int                             a_issue_age
+    ,int                             a_retirement_age
+    ,int                             a_inforce_duration
+    ,int                             a_effective_year
+    ,std::vector<std::string> const& a_allowed_keywords
+    ,bool                            a_keywords_only
+    )
+{
+    return SequenceParser
+        (input_expression
+        ,a_years_to_maturity
+        ,a_issue_age
+        ,a_retirement_age
+        ,a_inforce_duration
+        ,a_effective_year
+        ,a_allowed_keywords
+        ,a_keywords_only
+        );
+}
+
 /// Extract first substring from a '\n'-delimited exception::what().
 ///
 /// SequenceParser::diagnostics() returns a '\n'-delimited string
diff --git a/input_sequence_parser.hpp b/input_sequence_parser.hpp
index 4c92a03..b167cf2 100644
--- a/input_sequence_parser.hpp
+++ b/input_sequence_parser.hpp
@@ -27,20 +27,14 @@
 #include "config.hpp"
 
 #include "input_sequence_interval.hpp"
-#include "obstruct_slicing.hpp"
-#include "so_attributes.hpp"
-#include "uncopyable_lmi.hpp"
 
 #include <sstream>
 #include <string>
 #include <vector>
 
-class SequenceParser
-    :        private lmi::uncopyable <SequenceParser>
-    ,virtual private obstruct_slicing<SequenceParser>
+class SequenceParser final
 {
-  public:
-    SequenceParser
+    friend SequenceParser ParseSequence
         (std::string const&              input_expression
         ,int                             a_years_to_maturity
         ,int                             a_issue_age
@@ -51,12 +45,26 @@ class SequenceParser
         ,bool                            a_keywords_only
         );
 
+  public:
     ~SequenceParser();
 
-    std::string diagnostics() const;
+    std::string diagnostic_messages() const;
     std::vector<ValueInterval> const& intervals() const;
 
   private:
+    SequenceParser
+        (std::string const&              input_expression
+        ,int                             a_years_to_maturity
+        ,int                             a_issue_age
+        ,int                             a_retirement_age
+        ,int                             a_inforce_duration
+        ,int                             a_effective_year
+        ,std::vector<std::string> const& a_allowed_keywords
+        ,bool                            a_keywords_only
+        );
+    SequenceParser(SequenceParser const&);
+    SequenceParser& operator=(SequenceParser const&) = delete;
+
     enum token_type
         {e_eof             = 0
         ,e_major_separator = ';'
@@ -93,6 +101,9 @@ class SequenceParser
     void mark_diagnostic_context();
 
     std::istringstream input_stream_;
+    std::ostringstream diagnostics_;
+    std::string diagnostic_messages_;
+    std::vector<ValueInterval> intervals_;
 
     // Copies of ctor args that are identical to class InputSequence's.
     int years_to_maturity_;
@@ -103,6 +114,7 @@ class SequenceParser
     std::vector<std::string> allowed_keywords_;
     bool keywords_only_;
 
+    // Parser internals.
     token_type current_token_type_               = e_startup;
     double current_number_                       = 0.0;
     std::string current_keyword_;
@@ -111,11 +123,18 @@ class SequenceParser
     duration_mode current_duration_scalar_mode_  = e_inception;
     ValueInterval current_interval_;
     int last_input_duration_                     = 0;
-
-    std::ostringstream diagnostics_;
-
-    std::vector<ValueInterval> intervals_;
 };
 
+SequenceParser ParseSequence
+    (std::string const&              input_expression
+    ,int                             a_years_to_maturity
+    ,int                             a_issue_age
+    ,int                             a_retirement_age
+    ,int                             a_inforce_duration
+    ,int                             a_effective_year
+    ,std::vector<std::string> const& a_allowed_keywords
+    ,bool                            a_keywords_only
+    );
+
 #endif // input_sequence_parser_hpp
 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------