From: Ben Pfaff Date: Tue, 2 Oct 2007 04:13:07 +0000 (+0000) Subject: Fix bug #21192. Thanks to John Darrington for review. X-Git-Tag: v0.6.0~236 X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;ds=sidebyside;h=83ad8f5b760cb46bd1d86f60a40d6aace21118ef;p=pspp-builds.git 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. --- diff --git a/src/data/ChangeLog b/src/data/ChangeLog index d50a95da..819cb127 100644 --- a/src/data/ChangeLog +++ b/src/data/ChangeLog @@ -1,3 +1,19 @@ +2007-10-01 Ben Pfaff + + 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 Patch #6210. Reviewed by John Darrington. diff --git a/src/data/casereader.c b/src/data/casereader.c index 27c6148b..8b786456 100644 --- a/src/data/casereader.c +++ b/src/data/casereader.c @@ -56,19 +56,30 @@ static void insert_shim (struct casereader *); 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--; - return true; - } - else - { - reader->case_cnt = 0; - case_nullify (c); - return false; + if (reader->class->read (reader, reader->aux, c)) + { + assert (case_get_value_cnt (c) >= reader->value_cnt); + return true; + } } + reader->case_cnt = 0; + case_nullify (c); + return false; } /* Destroys READER. diff --git a/src/data/datasheet.c b/src/data/datasheet.c index f76b1908..4115cf02 100644 --- a/src/data/datasheet.c +++ b/src/data/datasheet.c @@ -24,6 +24,7 @@ #include #include #include +#include #include #include #include @@ -1258,6 +1259,93 @@ hash_datasheet (const struct datasheet *ds) 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 @@ check_datasheet (struct mc *mc, struct datasheet *ds, 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 @@ check_datasheet (struct mc *mc, struct datasheet *ds, 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_datasheet (struct mc *mc, struct datasheet *ds, } } + /* 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 @@ extract_data (const struct datasheet *ds, double data[MAX_ROWS][MAX_COLS]) 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); }