Re: [lmi] Delegating ctor uncalled for?

[Greg Chicares]

On 2017-02-04 02:16, Vadim Zeitlin wrote:
[...]
>  OTOH the current code still doesn't seem to be ideal to me because:
> 
> 1. It definitely looks like it ought to be possible to reuse the code
>    of the 3 ctors taking vectors in some way.

Please comment on this attempt. No comment is too trivial.

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/input_sequence.cpp b/input_sequence.cpp
index 101fa26..2433767 100644
--- a/input_sequence.cpp
+++ b/input_sequence.cpp
@@ -38,6 +38,7 @@
 #include <ostream>
 #include <sstream>
 #include <stdexcept>
+#include <type_traits>
 
 ValueInterval::ValueInterval()
     :value_number     (0.0)
@@ -797,50 +798,37 @@ InputSequence::InputSequence
 // SOMEDAY !! Ideally, therefore, they should be protected from
 // unintended use.
 
-InputSequence::InputSequence(std::vector<double> const& v)
-    :years_to_maturity_(v.size())
+// See commentary in header.
+void set_value(ValueInterval& v, double d)
 {
-    ValueInterval dummy;
-    dummy.value_is_keyword = false;
-
-    double prior_value = v.empty() ? 0.0 : v.front();
-    double current_value = prior_value;
-
-    intervals_.push_back(dummy);
-    intervals_.back().value_number = current_value;
-
-    for(auto const& vi : v)
-        {
-        current_value = vi;
-        if(prior_value == current_value)
-            {
-            ++intervals_.back().end_duration;
-            }
-        else
-            {
-            int value_change_duration = intervals_.back().end_duration;
-            intervals_.push_back(dummy);
-            intervals_.back().value_number = current_value;
-            intervals_.back().begin_duration = value_change_duration;
-            intervals_.back().end_duration = ++value_change_duration;
-            prior_value = current_value;
-            }
-        }
+    v.value_number = d;
+}
 
-    realize_vector();
+void set_value(ValueInterval& v, std::string const& s)
+{
+    v.value_keyword = s;
 }
 
-InputSequence::InputSequence(std::vector<std::string> const& v)
+template<typename T>
+InputSequence::InputSequence(std::vector<T> const& v)
     :years_to_maturity_(v.size())
 {
+    bool const T_is_double = std::is_same<T,double     >::value;
+    bool const T_is_string = std::is_same<T,std::string>::value;
+    static_assert(T_is_string || T_is_double, "");
+
     ValueInterval dummy;
-    dummy.value_is_keyword = true;
+    dummy.value_is_keyword = T_is_string;
 
-    std::string prior_value = v.empty() ? std::string() : v.front();
-    std::string current_value = prior_value;
+    T prior_value = v.empty() ? T() : v.front();
+    T current_value = prior_value;
 
     intervals_.push_back(dummy);
-    intervals_.back().value_keyword = current_value;
+// Can't do this because the "if" and "else" parts can't both compile:
+//  if(T_is_string) intervals_.back().value_keyword = current_value;
+//  else            intervals_.back().value_number  = current_value;
+// Instead of fixing that with std::enable_if, this seems cleaner:
+    intervals_.back().set_value(current_value);
 
     for(auto const& vi : v)
         {
@@ -853,7 +841,8 @@ InputSequence::InputSequence(std::vector<std::string> 
const& v)
             {
             int value_change_duration = intervals_.back().end_duration;
             intervals_.push_back(dummy);
-            intervals_.back().value_keyword = current_value;
+            intervals_.back().set_value(current_value);
+            set_value(intervals_.back(), current_value); // Hard to read!
             intervals_.back().begin_duration = value_change_duration;
             intervals_.back().end_duration = ++value_change_duration;
             prior_value = current_value;
@@ -863,6 +852,10 @@ InputSequence::InputSequence(std::vector<std::string> 
const& v)
     realize_vector();
 }
 
+// I couldn't figure out any simpler syntax that omits the arguments...
+template InputSequence::InputSequence(std::vector<double>      const&);
+template InputSequence::InputSequence(std::vector<std::string> const&);
+
 InputSequence::InputSequence
     (std::vector<double> const& n_v
     ,std::vector<std::string> const& s_v
diff --git a/input_sequence.hpp b/input_sequence.hpp
index e6fc244..74e6b27 100644
--- a/input_sequence.hpp
+++ b/input_sequence.hpp
@@ -158,6 +158,25 @@ struct ValueInterval
 {
     ValueInterval();
 
+// According to this style guide:
+//   https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
+// "structs should be used for passive objects that carry data, and may
+// have associated constants, but lack any functionality other than
+// access/setting the data members. The accessing/setting of fields is
+// done by directly accessing the fields rather than through method
+// invocations. Methods should not provide behavior but should only be
+// used to set up the data members, e.g., constructor, destructor,
+// Initialize(), Reset(), Validate()."
+//
+// OTOH, classes shouldn't have public nonconst data members.
+//
+// Alternatively, a free function set_value() is provided, and used in
+// the implementation for comparison, with the comment
+//   // Hard to read!
+
+    void set_value(double d)             {value_number  = d;}
+    void set_value(std::string const& s) {value_keyword = s;}
+
     double        value_number;
     std::string   value_keyword;
     bool          value_is_keyword;
@@ -267,8 +286,10 @@ class LMI_SO InputSequence
         ,std::string const&              a_default_keyword  = std::string()
         );
 
-    InputSequence(std::vector<double> const&);
-    InputSequence(std::vector<std::string> const&);
+    template<typename T>
+    InputSequence(std::vector<T> const& v);
+
+    // This should probably be removed: it doesn't seem at all useful:
     InputSequence(std::vector<double> const&, std::vector<std::string> const&);
 
     ~InputSequence();
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------