Re: OOP and load/save

[John W. Eaton]

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

| John W. Eaton wrote:
| > On 22-Apr-2009, Robert T. Short wrote:
| >
| > | I hadn't thought of that.  Good question and the answer is that there is 
| > | no guarantee.  In fact, some of my "real" classes require arguments. 
| > | 
| > | I think I have to go back to the drawing board on this. 
| >
| > I suggest that you try saving and reloading one of those classes
| > (preferably one with a parent class) in Matlab and see what happens.
| > Does it work?
| >
| > jwe
| >
| >
| >   
| Yes it works, but it occurs to me that I hadn't checked to see if all 
| the DETAILS worked right.  For example, I don't really know if the 
| aggregate class comes back as inherited.  Maybe more important, it 
| occurs to me right now that it may not even matter all that much.  It 
| may be difficult to actually test this, which stresses that maybe it 
| isn't as important as my perfectionist nature makes it out to be.
| 
| Food for thought.  It will take me some days to do this.  This was worth 
| the discussion.

You can start with the attached patch.  It seems to work for me, at
least for the simple things I tried.  Probably it still could use some
improvements.

jwe


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

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,24 @@
              }
            else
              {
+               std::list<std::string> parent_list;
+
+               if (n_fields > 0)
+                 {
+                   octave_value obj
+                     = octave_class::construct_object (classname);
+
+                   if (! error_state)
+                     parent_list = obj.parent_class_list ();
+                   else
+                     {
+                       // FIXME -- is there something better we can do here?
+                       error ("load: calling constructor for class `%s' with 
no arguments failed",
+                              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-base.h b/src/ov-base.h
--- a/src/ov-base.h
+++ b/src/ov-base.h
@@ -452,6 +452,9 @@
 
   virtual string_vector map_keys (void) const;
 
+  virtual std::list<std::string> parent_class_list (void) const
+    { return std::list<std::string> (); }
+
   virtual string_vector parent_class_names (void) const
     { return string_vector (); }
 
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,75 @@
        {
          if (len > 0)
            {
-             Octave_map m (map);
+             octave_value obj = construct_object (classname);
 
-             for (octave_idx_type j = 0; j < len; j++)
+             if (! error_state)
                {
-                 octave_value t2;
-                 bool dummy;
+                 parent_list = obj.parent_class_list ();
 
-                 // recurse to read cell elements
-                 std::string nm
-                   = read_ascii_data (is, std::string (), dummy, t2, j);
+                 Octave_map m (obj.map_value ());
 
-                 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 (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
                    {
-                     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: failed to load class");
+                     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 ();
+           {
+             // FIXME -- is there something better we can do here?
+             error ("load: calling constructor for class `%s' with no 
arguments failed",
+                    classname.c_str ());
+             success = false;
+           }
        }
       else 
        {
@@ -963,6 +999,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 +1024,68 @@
 
   if (len > 0)
     {
-      Octave_map m (map);
+      octave_value obj = construct_object (c_name);
 
-      for (octave_idx_type j = 0; j < len; j++)
+      if (! error_state)
        {
-         octave_value t2;
-         bool dummy;
-         std::string doc;
+         parent_list = obj.parent_class_list ();
 
-         // recurse to read cell elements
-         std::string nm = read_binary_data (is, swap, fmt, std::string (), 
-                                            dummy, t2, doc);
+         Octave_map m (obj.map_value ());
 
-         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 (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
            {
-             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: failed to load class");
+             success = false;
            }
        }
       else
        {
-         error ("load: failed to load class");
+         // FIXME -- is there something better we can do here?
+         error ("load: calling constructor for class `%s' with no arguments 
failed",
+                c_name.c_str ());
          success = false;
        }
     }
-  else if (len == 0 )
+  else if (len == 0)
     map = Octave_map (dim_vector (1, 1));
   else
     panic_impossible ();
@@ -1129,6 +1182,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 +1208,6 @@
   if (group_hid < 0)
     goto error_cleanup;
 
-  
   data_hid = H5Dopen (group_hid, "classname");
 
   if (data_hid < 0)
@@ -1201,7 +1256,6 @@
     }
   while (0);
 
-
 #ifdef HAVE_H5GGET_NUM_OBJS
   subgroup_hid = H5Gopen (group_hid, name); 
   H5Gget_num_objs (subgroup_hid, &num_obj);
@@ -1233,23 +1287,39 @@
 
   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_value obj = construct_object (c_name);
 
          if (! error_state)
+           parent_list = obj.parent_class_list ();
+         else
            {
-             map = tmp(0).map_value ();
-             retval = true;
+             // FIXME -- is there something better we can do here?
+             error ("load: calling constructor for class `%s' with no 
arguments failed",
+                    c_name.c_str ());
+             retval = false;
+             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:
diff --git a/src/ov-class.h b/src/ov-class.h
--- a/src/ov-class.h
+++ b/src/ov-class.h
@@ -125,6 +125,9 @@
 
   string_vector map_keys (void) const;
 
+  std::list<std::string> parent_class_list (void) const
+    { return parent_list; }
+
   string_vector parent_class_names (void) const
     { return string_vector (parent_list); }
 
@@ -156,6 +159,8 @@
 
   mxArray *as_mxArray (void) const;
 
+  static octave_value construct_object (const std::string& c_name);
+
 private:
 
   Octave_map map;
diff --git a/src/ov.h b/src/ov.h
--- a/src/ov.h
+++ b/src/ov.h
@@ -830,6 +830,9 @@
   string_vector map_keys (void) const
     { return rep->map_keys (); }
 
+  std::list<std::string> parent_class_list (void) const
+    { return rep->parent_class_list (); }
+
   string_vector parent_class_names (void) const
     { return rep->parent_class_names (); }