[Pspp-cvs] pspp/src/data datasheet.c casereader.c ChangeLog

[Ben Pfaff]

CVSROOT:        /cvsroot/pspp
Module name:    pspp
Changes by:     Ben Pfaff <blp> 07/10/02 04:13:07

Modified files:
        src/data       : datasheet.c casereader.c ChangeLog 

Log message:
        Fix bug #21192.  Thanks to John Darrington for review.
        
        * casereader.c (casereader_read): Decrement case_cnt before
        calling the casereader's "read" member function, so that we
        interact properly with lazy_casereader.
        
        * datasheet.c: Add regression test for above bug fix.
        (clone_datasheet): New function.
        (lazy_callback): New function.
        (check_datasheet_casereader): New function.
        (check_datasheet): Check datasheet contents are reported correctly
        through an ordinary casereader and a lazy casereader.
        (clone_model): Use clone_datasheet.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/datasheet.c?cvsroot=pspp&r1=1.8&r2=1.9
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/casereader.c?cvsroot=pspp&r1=1.7&r2=1.8
http://cvs.savannah.gnu.org/viewcvs/pspp/src/data/ChangeLog?cvsroot=pspp&r1=1.161&r2=1.162

Patches:
Index: datasheet.c
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/datasheet.c,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -b -r1.8 -r1.9
--- datasheet.c 13 Sep 2007 12:37:09 -0000      1.8
+++ datasheet.c 2 Oct 2007 04:13:07 -0000       1.9
@@ -24,6 +24,7 @@
 #include <data/casereader-provider.h>
 #include <data/casereader.h>
 #include <data/casewriter.h>
+#include <data/lazy-casereader.h>
 #include <data/sparse-cases.h>
 #include <libpspp/array.h>
 #include <libpspp/assertion.h>
@@ -1258,6 +1259,93 @@
   return hash[0];
 }
 
+/* Clones the structure and contents of ODS into a new datasheet,
+   and returns the new datasheet. */
+static struct datasheet *
+clone_datasheet (const struct datasheet *ods)
+{
+  struct datasheet *ds;
+  struct range_map_node *r;
+
+  ds = xmalloc (sizeof *ds);
+  ds->columns = axis_clone (ods->columns);
+  ds->rows = axis_clone (ods->rows);
+  range_map_init (&ds->sources);
+  for (r = range_map_first (&ods->sources); r != NULL;
+       r = range_map_next (&ods->sources, r))
+    {
+      const struct source_info *osi = source_info_from_range_map (r);
+      struct source_info *si = xmalloc (sizeof *si);
+      si->source = source_clone (osi->source);
+      range_map_insert (&ds->sources, range_map_node_get_start (r),
+                        range_map_node_get_width (r), &si->column_range);
+    }
+  ds->column_min_alloc = ods->column_min_alloc;
+  ds->taint = taint_create ();
+
+  return ds;
+}
+
+/* lazy_casereader callback function to instantiate a casereader
+   from the datasheet. */
+static struct casereader *
+lazy_callback (void *ds_)
+{
+  struct datasheet *ds = ds_;
+  return datasheet_make_reader (ds);
+}
+
+/* Checks that READER contains the ROW_CNT rows and COLUMN_CNT
+   columns of data in ARRAY, reporting any errors via MC. */
+static void
+check_datasheet_casereader (struct mc *mc, struct casereader *reader,
+                            double array[MAX_ROWS][MAX_COLS],
+                            size_t row_cnt, size_t column_cnt)
+{
+  if (casereader_get_case_cnt (reader) != row_cnt)
+    {
+      if (casereader_get_case_cnt (reader) == CASENUMBER_MAX
+          && casereader_count_cases (reader) == row_cnt)
+        mc_error (mc, "datasheet casereader has unknown case count");
+      else
+        mc_error (mc, "casereader row count (%lu) does not match "
+                  "expected (%zu)",
+                  (unsigned long int) casereader_get_case_cnt (reader),
+                  row_cnt);
+    }
+  else if (casereader_get_value_cnt (reader) != column_cnt)
+    mc_error (mc, "casereader column count (%zu) does not match "
+              "expected (%zu)",
+              casereader_get_value_cnt (reader), column_cnt);
+  else
+    {
+      struct ccase c;
+      size_t row;
+
+      for (row = 0; row < row_cnt; row++)
+        {
+          size_t col;
+
+          if (!casereader_read (reader, &c))
+            {
+              mc_error (mc, "casereader_read failed reading row %zu of %zu "
+                        "(%zu columns)", row, row_cnt, column_cnt);
+              return;
+            }
+
+          for (col = 0; col < column_cnt; col++)
+            if (case_num_idx (&c, col) != array[row][col])
+              mc_error (mc, "element %zu,%zu (of %zu,%zu) differs: "
+                        "%g != %g",
+                        row, col, row_cnt, column_cnt,
+                        case_num_idx (&c, col), array[row][col]);
+        }
+
+      if (casereader_read (reader, &c))
+        mc_error (mc, "casereader has extra cases (expected %zu)", row_cnt);
+    }
+}
+
 /* Checks that datasheet DS contains has ROW_CNT rows, COLUMN_CNT
    columns, and the same contents as ARRAY, reporting any
    mismatches via mc_error.  Then, adds DS to MC as a new state. */
@@ -1266,6 +1354,10 @@
                  double array[MAX_ROWS][MAX_COLS],
                  size_t row_cnt, size_t column_cnt)
 {
+  struct datasheet *ds2;
+  struct casereader *reader;
+  unsigned long int serial = 0;
+
   assert (row_cnt < MAX_ROWS);
   assert (column_cnt < MAX_COLS);
 
@@ -1277,6 +1369,7 @@
       return;
     }
 
+  /* Check contents of datasheet via datasheet functions. */
   if (row_cnt != datasheet_get_row_cnt (ds))
     mc_error (mc, "row count (%lu) does not match expected (%zu)",
               (unsigned long int) datasheet_get_row_cnt (ds), row_cnt);
@@ -1299,6 +1392,43 @@
           }
     }
 
+  /* Check that datasheet contents are correct when read through
+     casereader. */
+  ds2 = clone_datasheet (ds);
+  reader = datasheet_make_reader (ds2);
+  check_datasheet_casereader (mc, reader, array, row_cnt, column_cnt);
+  casereader_destroy (reader);
+
+  /* Check that datasheet contents are correct when read through
+     casereader with lazy_casereader wrapped around it.  This is
+     valuable because otherwise there is no non-GUI code that
+     uses the lazy_casereader. */
+  ds2 = clone_datasheet (ds);
+  reader = lazy_casereader_create (column_cnt, row_cnt,
+                                   lazy_callback, ds2, &serial);
+  check_datasheet_casereader (mc, reader, array, row_cnt, column_cnt);
+  if (lazy_casereader_destroy (reader, serial))
+    {
+      /* Lazy casereader was never instantiated.  This will
+         only happen if there are no rows (because in that case
+         casereader_read never gets called). */
+      datasheet_destroy (ds2);
+      if (row_cnt != 0)
+        mc_error (mc, "lazy casereader not instantiated, but should "
+                  "have been (size %zu,%zu)", row_cnt, column_cnt);
+    }
+  else
+    {
+      /* Lazy casereader was instantiated.  This is the common
+         case, in which some casereader operation
+         (casereader_read in this case) was performed on the
+         lazy casereader. */
+      casereader_destroy (reader);
+      if (row_cnt == 0)
+        mc_error (mc, "lazy casereader instantiated, but should not "
+                  "have been (size %zu,%zu)", row_cnt, column_cnt);
+    }
+
   mc_add_state (mc, ds);
 }
 
@@ -1326,29 +1456,9 @@
    and the contents of ODATA into DATA. */
 static void
 clone_model (const struct datasheet *ods, double odata[MAX_ROWS][MAX_COLS],
-             struct datasheet **ds_, double data[MAX_ROWS][MAX_COLS])
+             struct datasheet **ds, double data[MAX_ROWS][MAX_COLS])
 {
-  struct datasheet *ds;
-  struct range_map_node *r;
-
-  /* Clone ODS into DS. */
-  ds = *ds_ = xmalloc (sizeof *ds);
-  ds->columns = axis_clone (ods->columns);
-  ds->rows = axis_clone (ods->rows);
-  range_map_init (&ds->sources);
-  for (r = range_map_first (&ods->sources); r != NULL;
-       r = range_map_next (&ods->sources, r))
-    {
-      const struct source_info *osi = source_info_from_range_map (r);
-      struct source_info *si = xmalloc (sizeof *si);
-      si->source = source_clone (osi->source);
-      range_map_insert (&ds->sources, range_map_node_get_start (r),
-                        range_map_node_get_width (r), &si->column_range);
-    }
-  ds->column_min_alloc = ods->column_min_alloc;
-  ds->taint = taint_create ();
-
-  /* Clone ODATA into DATA. */
+  *ds = clone_datasheet (ods);
   memcpy (data, odata, MAX_ROWS * MAX_COLS * sizeof **data);
 }
 

Index: casereader.c
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/casereader.c,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -b -r1.7 -r1.8
--- casereader.c        19 Sep 2007 04:29:00 -0000      1.7
+++ casereader.c        2 Oct 2007 04:13:07 -0000       1.8
@@ -56,19 +56,30 @@
 bool
 casereader_read (struct casereader *reader, struct ccase *c)
 {
-  if (reader->case_cnt != 0 && reader->class->read (reader, reader->aux, c))
+  if (reader->case_cnt != 0)
     {
-      assert (case_get_value_cnt (c) >= reader->value_cnt);
+      /* ->read may use casereader_swap to replace itself by
+         another reader and then delegate to that reader by
+         recursively calling casereader_read.  Currently only
+         lazy_casereader does this and, with luck, nothing else
+         ever will.
+
+         To allow this to work, however, we must decrement
+         case_cnt before calling ->read.  If we decremented
+         case_cnt after calling ->read, then this would actually
+         drop two cases from case_cnt instead of one, and we'd
+         lose the last case in the casereader. */
       if (reader->case_cnt != CASENUMBER_MAX)
         reader->case_cnt--;
+      if (reader->class->read (reader, reader->aux, c))
+        {
+          assert (case_get_value_cnt (c) >= reader->value_cnt);
       return true;
     }
-  else
-    {
+    }
       reader->case_cnt = 0;
       case_nullify (c);
       return false;
-    }
 }
 
 /* Destroys READER.

Index: ChangeLog
===================================================================
RCS file: /cvsroot/pspp/pspp/src/data/ChangeLog,v
retrieving revision 1.161
retrieving revision 1.162
diff -u -b -r1.161 -r1.162
--- ChangeLog   25 Sep 2007 04:26:25 -0000      1.161
+++ ChangeLog   2 Oct 2007 04:13:07 -0000       1.162
@@ -1,3 +1,19 @@
+2007-10-01  Ben Pfaff  <address@hidden>
+
+       Fix bug #21192.  Thanks to John Darrington for review.
+
+       * casereader.c (casereader_read): Decrement case_cnt before
+       calling the casereader's "read" member function, so that we
+       interact properly with lazy_casereader.
+
+       * datasheet.c: Add regression test for above bug fix.
+       (clone_datasheet): New function.
+       (lazy_callback): New function.
+       (check_datasheet_casereader): New function.
+       (check_datasheet): Check datasheet contents are reported correctly
+       through an ordinary casereader and a lazy casereader.
+       (clone_model): Use clone_datasheet.
+
 2007-09-24  Ben Pfaff  <address@hidden>
 
        Patch #6210.  Reviewed by John Darrington.