Re: OOP and load/save

[John W. Eaton]

On 23-Apr-2009, Robert T. Short wrote:

| John, let me make a proposal.  I think I can do a fairly quick fix that 
| is ALMOST correct.  That way, folks (like me) can use the OOP facilities 
| without worrying about long-term breakage.  Then I (or you or whoever) 
| can take the time to really think about a correct implementation.

I'd really prefer to try to get a correct and more or less complete
implementation from the start.

| Suppose that I read the .mat file without concerning myself about the 
| parent structure.  This involves simply creating an octave_class, 
| setting the class name, and then creating a octave_map, I think 
| (actually, I recall Cells being used, but I think we can still do this).
| 
| Then I can walk through the elements in the map.  IF the map elements 
| are objects AND the object's class name is the same as the name of the 
| struct field, just assume that I have a derived class.  Add it to the 
| parent list for the class.  This has to recurse of course for multiple 
| levels of inheritance.

Instead of faking it, let's keep track of the classes that we have
constructed, and then look at those for the parent and field info.
Then if there is a mismatch, we can report it.  If an exemplar doesn't
exist, you can call the constructor to attempt to create one.

I've checked in a patch that implements the storing of exemplars, and
checks newly constructed objects against them, so if the constructor
changes and the object is different, we will now get an error (a
diagnostic that was previously missing in Octave).  The patch for this
is here:

  http://hg.savannah.gnu.org/hgweb/octave/rev/d8f9588c6ba1

I've also updated my previous patch to look for objects in the
exemplar map before attempting to call the constructor.  My current
draft is attached below.  I haven't checked it in since it is still
not complete, and I think it should use some more testing.

We also still need to implement things like "clear classes".

jwe

# HG changeset patch
# User John W. Eaton <address@hidden>
# Date 1240522081 14400
# Node ID b3c8b675e8478854a14fdc14b5b3bab54d85f8cd
# Parent  d8f9588c6ba1d6fdbedf783847092fedad8397e1
use constructor to load parent class info for objects

diff --git a/liboctave/str-vec.cc b/liboctave/str-vec.cc
--- a/liboctave/str-vec.cc
+++ b/liboctave/str-vec.cc
@@ -238,6 +238,37 @@
   return os;
 }
 
+bool
+operator == (const string_vector& a, const string_vector& b)
+{
+  bool retval = true;
+
+  octave_idx_type a_len = a.numel ();
+  octave_idx_type b_len = b.numel ();
+
+  if (a_len == b_len)
+    {
+      for (octave_idx_type i = 0; i < a_len; i++)
+       {
+         if (a[i] != b[i])
+           {
+             retval = false;
+             break;
+           }
+       }
+    }
+  else
+    retval = false;
+
+  return retval;
+}
+
+bool
+operator != (const string_vector& a, const string_vector& b)
+{
+  return ! (a == b);
+}
+
 /*
 ;;; Local Variables: ***
 ;;; mode: C++ ***
diff --git a/liboctave/str-vec.h b/liboctave/str-vec.h
--- a/liboctave/str-vec.h
+++ b/liboctave/str-vec.h
@@ -99,6 +99,12 @@
   std::ostream& list_in_columns (std::ostream&, int width = 0) const;
 };
 
+bool
+operator == (const string_vector& a, const string_vector& b);
+
+bool
+operator != (const string_vector& a, const string_vector& b);
+
 #endif
 
 /*
diff --git a/src/ls-mat5.cc b/src/ls-mat5.cc
--- a/src/ls-mat5.cc
+++ b/src/ls-mat5.cc
@@ -1106,6 +1106,27 @@
              }
            else
              {
+               std::list<std::string> parent_list;
+
+               if (n_fields > 0)
+                 {
+                   octave_class::exemplar_info info
+                     = octave_class::get_object_info (classname);
+
+                   if (! error_state)
+                     parent_list = info.parents ();
+                   else
+                     goto data_read_error;
+
+                   if (m.keys () != info.fields ())
+                     {
+                       // FIXME -- is there something better we can do here?
+                       error ("load: mismatch in field names for class `%s'",
+                              classname.c_str ());
+                       goto data_read_error;
+                     }
+                 }
+
                tc = new octave_class (m, classname);
 
                if (load_path::find_method (classname, "loadobj") != 
diff --git a/src/ov-class.cc b/src/ov-class.cc
--- a/src/ov-class.cc
+++ b/src/ov-class.cc
@@ -831,6 +831,28 @@
   return true;
 }
 
+octave_value
+octave_class::construct_object (const std::string& classname)
+{
+  octave_value retval;
+
+  octave_value ctor = symbol_table::find_method (classname, classname);
+
+  if (ctor.is_defined ())
+    {
+      octave_value_list result = feval (ctor, 1);
+
+      if (result.length () == 1)
+       retval = result(0);
+      else
+       error ("call to constructor for class %s failed", classname.c_str ());
+    }
+  else
+    error ("no constructor for class %s", classname.c_str ());
+
+  return retval;
+}
+
 bool 
 octave_class::load_ascii (std::istream& is)
 {
@@ -844,61 +866,79 @@
        {
          if (len > 0)
            {
-             Octave_map m (map);
+             octave_class::exemplar_info info = get_object_info (classname);
 
-             for (octave_idx_type j = 0; j < len; j++)
+             if (! error_state)
                {
-                 octave_value t2;
-                 bool dummy;
+                 parent_list = info.parents ();
 
-                 // recurse to read cell elements
-                 std::string nm
-                   = read_ascii_data (is, std::string (), dummy, t2, j);
+                 Octave_map m;
 
-                 if (!is)
-                   break;
+                 for (octave_idx_type j = 0; j < len; j++)
+                   {
+                     octave_value t2;
+                     bool dummy;
 
-                 Cell tcell = t2.is_cell () ? t2.cell_value () : Cell (t2);
+                     // recurse to read cell elements
+                     std::string nm
+                       = read_ascii_data (is, std::string (), dummy, t2, j);
 
-                 if (error_state)
-                   {
-                     error ("load: internal error loading class elements");
-                     return false;
+                     if (!is)
+                       break;
+
+                     Cell tcell = t2.is_cell () ? t2.cell_value () : Cell (t2);
+
+                     if (error_state)
+                       {
+                         error ("load: internal error loading class elements");
+                         return false;
+                       }
+
+                     m.assign (nm, tcell);
                    }
 
-                 m.assign (nm, tcell);
-               }
+                 if (m.keys () == info.fields ())
+                   {
+                     if (is) 
+                       {
+                         map = m;
+                         c_name = classname;
 
-             if (is) 
-               {
-                 map = m;
-                 c_name = classname;
+                         if (load_path::find_method (classname, "loadobj") != 
+                             std::string())
+                           {
+                             octave_value in = new octave_class (*this);
+                             octave_value_list tmp = feval ("loadobj", in, 1);
 
-                 if (load_path::find_method (classname, "loadobj") != 
-                     std::string())
+                             if (! error_state)
+                               map = tmp(0).map_value ();
+                             else
+                               success = false;
+                           }
+                       }
+                     else
+                       {
+                         error ("load: failed to load class");
+                         success = false;
+                       }
+                   }
+                 else
                    {
-                     octave_value in = new octave_class (*this);
-                     octave_value_list tmp = feval ("loadobj", in, 1);
-
-                     if (! error_state)
-                       map = tmp(0).map_value ();
-                     else
-                       success = false;
+                     error ("load: mismatch in field names for class `%s'",
+                            classname.c_str ());
+                     success = false;
                    }
                }
+             else if (len == 0)
+               {
+                 map = Octave_map (dim_vector (1, 1));
+                 c_name = classname;
+               }
              else
-               {
-                 error ("load: failed to load class");
-                 success = false;
-               }
-           }
-         else if (len == 0 )
-           {
-             map = Octave_map (dim_vector (1, 1));
-             c_name = classname;
+               panic_impossible ();
            }
          else
-           panic_impossible ();
+           success = false;
        }
       else 
        {
@@ -963,6 +1003,9 @@
 
   int32_t classname_len;
 
+  // FIXME -- we could use some diagnostics here if reading various
+  // elements fails.  See the messages in load_ascii.
+
   is.read (reinterpret_cast<char *> (&classname_len), 4);
   if (! is)
     return false;
@@ -985,54 +1028,72 @@
 
   if (len > 0)
     {
-      Octave_map m (map);
+      octave_class::exemplar_info info = get_object_info (c_name);
 
-      for (octave_idx_type j = 0; j < len; j++)
+      if (! error_state)
        {
-         octave_value t2;
-         bool dummy;
-         std::string doc;
+         parent_list = info.parents ();
 
-         // recurse to read cell elements
-         std::string nm = read_binary_data (is, swap, fmt, std::string (), 
-                                            dummy, t2, doc);
+         Octave_map m;
 
-         if (!is)
-           break;
+         for (octave_idx_type j = 0; j < len; j++)
+           {
+             octave_value t2;
+             bool dummy;
+             std::string doc;
 
-         Cell tcell = t2.is_cell () ? t2.cell_value () : Cell (t2);
- 
-         if (error_state)
-           {
-             error ("load: internal error loading class elements");
-             return false;
+             // recurse to read cell elements
+             std::string nm = read_binary_data (is, swap, fmt, std::string (), 
+                                                dummy, t2, doc);
+
+             if (!is)
+               break;
+
+             Cell tcell = t2.is_cell () ? t2.cell_value () : Cell (t2);
+
+             if (error_state)
+               {
+                 error ("load: internal error loading class elements");
+                 return false;
+               }
+
+             m.assign (nm, tcell);
            }
 
-         m.assign (nm, tcell);
-       }
+         if (m.keys () == info.fields ())
+           {
+             if (is) 
+               {
+                 map = m;
 
-      if (is) 
-       {
-         map = m;
+                 if (load_path::find_method (class_name(), "loadobj") != 
std::string())
+                   {
+                     octave_value in = new octave_class (*this);
+                     octave_value_list tmp = feval ("loadobj", in, 1);
 
-         if (load_path::find_method (class_name(), "loadobj") != std::string())
+                     if (! error_state)
+                       map = tmp(0).map_value ();
+                     else
+                       success = false;
+                   }
+               }
+             else
+               {
+                 error ("load: failed to load class");
+                 success = false;
+               }
+           }
+         else
            {
-             octave_value in = new octave_class (*this);
-             octave_value_list tmp = feval ("loadobj", in, 1);
-
-             if (! error_state)
-               map = tmp(0).map_value ();
-             else
-               success = false;
+             error ("load: mismatch in field names for class `%s'",
+                    c_name.c_str ());
+             success = false;
            }
        }
       else
-       {
-         error ("load: failed to load class");
-         success = false;
-       }
+       success = false;
     }
-  else if (len == 0 )
+  else if (len == 0)
     map = Octave_map (dim_vector (1, 1));
   else
     panic_impossible ();
@@ -1129,6 +1190,9 @@
 octave_class::load_hdf5 (hid_t loc_id, const char *name,
                          bool have_h5giterate_bug)
 {
+  // FIXME -- this function could use more diagnostics, similar to
+  // what we have in load_ascii.
+
   bool retval = false;
 
   hid_t group_hid = -1;
@@ -1152,7 +1216,6 @@
   if (group_hid < 0)
     goto error_cleanup;
 
-  
   data_hid = H5Dopen (group_hid, "classname");
 
   if (data_hid < 0)
@@ -1201,7 +1264,6 @@
     }
   while (0);
 
-
 #ifdef HAVE_H5GGET_NUM_OBJS
   subgroup_hid = H5Gopen (group_hid, name); 
   H5Gget_num_objs (subgroup_hid, &num_obj);
@@ -1233,23 +1295,46 @@
 
   if (retval2 >= 0)
     {
-      map = m;
-
-      if (load_path::find_method (class_name(), "loadobj") != std::string())
+      if (retval2 > 0)
        {
-         octave_value in = new octave_class (*this);
-         octave_value_list tmp = feval ("loadobj", in, 1);
+         octave_class::exemplar_info info = get_object_info (c_name);
 
          if (! error_state)
+           parent_list = info.parents ();
+         else
            {
-             map = tmp(0).map_value ();
-             retval = true;
+             // FIXME -- is there something better we can do here?
+             retval = false;
+             goto error_cleanup;
+           }
+
+         if (m.keys () != info.fields ())
+           {
+             // FIXME -- is there something better we can do here?
+             retval = false;
+             error ("load: mismatch in field names for class `%s'",
+                    c_name.c_str ());
+             goto error_cleanup;
+           }
+
+         map = m;
+
+         if (load_path::find_method (class_name(), "loadobj") != std::string())
+           {
+             octave_value in = new octave_class (*this);
+             octave_value_list tmp = feval ("loadobj", in, 1);
+
+             if (! error_state)
+               {
+                 map = tmp(0).map_value ();
+                 retval = true;
+               }
+             else
+               retval = false;
            }
          else
-           retval = false;
+           retval = true;
        }
-      else
-       retval = true;
     }
   
  error_cleanup:
@@ -1295,7 +1380,7 @@
       parent_class_names = obj.parent_class_name_list ();
     }
   else
-    error ("invalid call to exmplar_info constructor");
+    error ("invalid call to exemplar_info constructor");
 }
 
 
@@ -1315,32 +1400,31 @@
          string_vector obj_fnames = obj_map.keys ();
          string_vector fnames = fields ();
 
-         for (octave_idx_type i = 0; i < nfields (); i++)
+         if (obj_fnames != fnames)
            {
-             if (obj_fnames[i] != fnames[i])
-               {
-                 retval = false;
-                 error ("mismatch in field names");
-                 break;
-               }
+             retval= false;
+             error ("mismatch in field names");
            }
 
-         if (nparents () == obj.nparents ())
+         if (! error_state)
            {
-             std::list<std::string> obj_parents
-               = obj.parent_class_name_list ();
-             std::list<std::string> pnames = parents ();
+             if (nparents () == obj.nparents ())
+               {
+                 std::list<std::string> obj_parents
+                   = obj.parent_class_name_list ();
+                 std::list<std::string> pnames = parents ();
 
-             std::list<std::string>::const_iterator p = obj_parents.begin ();
-             std::list<std::string>::const_iterator q = pnames.begin ();
+                 std::list<std::string>::const_iterator p = obj_parents.begin 
();
+                 std::list<std::string>::const_iterator q = pnames.begin ();
 
-             while (p != obj_parents.end ())
-               {
-                 if (*p++ != *q++)
+                 while (p != obj_parents.end ())
                    {
-                     retval = false;
-                     error ("mismatch in parent classes");
-                     break;
+                     if (*p++ != *q++)
+                       {
+                         retval = false;
+                         error ("mismatch in parent classes");
+                         break;
+                       }
                    }
                }
            }
@@ -1365,6 +1449,37 @@
   return retval;
 }
 
+octave_class::exemplar_info
+octave_class::get_object_info (const std::string& classname)
+{
+  exemplar_info retval;
+
+  exemplar_map_const_iterator p = exemplar_map.find (classname);
+
+  if (p == exemplar_map.end ())
+    {
+      construct_object (classname);
+
+      if (error_state)
+       {
+         error ("calling constructor for class `%s' with no arguments failed",
+                classname.c_str ());
+
+         return retval;
+       }
+
+      p = exemplar_map.find (classname);
+    }
+
+  if (p != exemplar_map.end ())
+    retval = p->second;
+  else
+    error ("unable to determine properties of class `%s'",
+          classname.c_str ());
+
+  return retval;
+}
+
 DEFUN (class, args, ,
   "-*- texinfo -*-\n\
 @deftypefn {Built-in Function} {} class (@var{expr})\n\
@@ -1409,7 +1524,7 @@
 
                  if (! error_state)
                    {
-                     octave_class::exemplar_const_iterator it
+                     octave_class::exemplar_map_const_iterator it
                        = octave_class::exemplar_map.find (id);
 
                      if (it == octave_class::exemplar_map.end ())
diff --git a/src/ov-class.h b/src/ov-class.h
--- a/src/ov-class.h
+++ b/src/ov-class.h
@@ -161,6 +161,8 @@
 
   mxArray *as_mxArray (void) const;
 
+  static octave_value construct_object (const std::string& c_name);
+
 private:
 
   Octave_map map;
@@ -230,8 +232,10 @@
   // A map from class names to lists of fields.
   static std::map<std::string, exemplar_info> exemplar_map;
 
-  typedef std::map<std::string, exemplar_info>::iterator exemplar_iterator;
-  typedef std::map<std::string, exemplar_info>::const_iterator 
exemplar_const_iterator;
+  typedef std::map<std::string, exemplar_info>::iterator exemplar_map_iterator;
+  typedef std::map<std::string, exemplar_info>::const_iterator 
exemplar_map_const_iterator;
+
+  static exemplar_info get_object_info (const std::string& classname);
 };
 
 #endif