[gnumeric] Criteria: cleanup.

[Morten Welinder <mortenw src gnome org>]

commit 45ddf040faf636ada295a2115bf34fc22b7dc08f
Author: Morten Welinder <terra gnome org>
Date:   Fri Jun 19 16:57:49 2009 -0400

    Criteria: cleanup.

 ChangeLog                   |    5 ++
 plugins/fn-math/functions.c |  123 +++++++++++++++++++++++--------------------
 src/value.c                 |   99 ++++++++++++++++++-----------------
 src/value.h                 |   11 ++--
 4 files changed, 127 insertions(+), 111 deletions(-)
---
diff --git a/ChangeLog b/ChangeLog
index 17a1db7..bde40c4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-06-19  Morten Welinder  <terra gnome org>
+
+	* src/value.c (parse_criteria): Return a GnmCriteria.  All callers
+	changed.
+
 2009-06-18  Morten Welinder  <terra gnome org>
 
 	* src/func.c (function_call_with_exprs): Make sure we can tell
diff --git a/plugins/fn-math/functions.c b/plugins/fn-math/functions.c
index 1367d22..ab8ba5f 100644
--- a/plugins/fn-math/functions.c
+++ b/plugins/fn-math/functions.c
@@ -582,23 +582,29 @@ static GnmFuncHelp const help_countif[] = {
 };
 
 typedef struct {
-        GnmCriteriaFunc  test;
-        GnmValue *test_value;
-	GODateConventions const *date_conv;
+	GnmCriteria *crit;
 	int count;
 } CountIfClosure;
 
 static GnmValue *
 cb_countif (GnmCellIter const *iter, CountIfClosure *res)
 {
-	GnmCell *cell;
-	if (NULL != (cell = iter->cell)) {
-		gnm_cell_eval (cell);
-		if ((VALUE_IS_NUMBER (cell->value) ||
-		     VALUE_IS_STRING (cell->value)) &&
-		    (res->test) (cell->value, res->test_value, res->date_conv))
-			res->count++;
-	}
+	GnmCell *cell = iter->cell;
+	GnmValue *v;
+
+	if (!cell)
+		return NULL;
+
+	gnm_cell_eval (cell);
+	v = cell->value;
+
+	if (!VALUE_IS_NUMBER (v) && !VALUE_IS_STRING (v))
+		return NULL;
+
+	if (!res->crit->fun (v, res->crit->x, res->crit->date_conv))
+		return NULL;
+
+	res->count++;
 
 	return NULL;
 }
@@ -609,8 +615,9 @@ gnumeric_countif (GnmFuncEvalInfo *ei, GnmValue const * const *argv)
         GnmValueRange const *r = &argv[0]->v_range;
 	Sheet		*sheet;
 	GnmValue        *problem;
-	CellIterFlags	 iter_flags;
 	CountIfClosure   res;
+	GODateConventions const *date_conv =
+		workbook_date_conv (ei->pos->sheet->workbook);
 
 	/* XL has some limitations on @range that we currently emulate, but do
 	 * not need to.
@@ -622,18 +629,18 @@ gnumeric_countif (GnmFuncEvalInfo *ei, GnmValue const * const *argv)
 	    (!VALUE_IS_NUMBER (argv[1]) && !VALUE_IS_STRING (argv[1])))
 	        return value_new_error_VALUE (ei->pos);
 
-	res.date_conv = sheet ?	workbook_date_conv (sheet->workbook) : NULL;
-
 	res.count = 0;
-	parse_criteria (argv[1], &res.test, &res.test_value, &iter_flags,
-		workbook_date_conv (ei->pos->sheet->workbook));
-#warning 2006/May/31  Why do we not filter non-existent as a flag, rather than checking for NULL in cb_countif
-	problem = sheet_foreach_cell_in_range (sheet, iter_flags,
-		r->cell.a.col, r->cell.a.row, r->cell.b.col, r->cell.b.row,
-		(CellIterFunc) &cb_countif, &res);
-	value_release (res.test_value);
+	res.crit = parse_criteria (argv[1], date_conv);
+#warning "2006/May/31  Why do we not filter non-existent as a flag, rather than checking for NULL in cb_countif"
+	problem = sheet_foreach_cell_in_range
+		(sheet, res.crit->iter_flags,
+		 r->cell.a.col, r->cell.a.row, r->cell.b.col, r->cell.b.row,
+		 (CellIterFunc) &cb_countif, &res);
+	free_criteria (res.crit);
+
 	if (NULL != problem)
 	        return value_new_error_VALUE (ei->pos);
+
 	return value_new_int (res.count);
 }
 
@@ -666,45 +673,47 @@ static GnmFuncHelp const help_sumif[] = {
 };
 
 typedef struct {
-        GnmCriteriaFunc  test;
-        GnmValue            *test_value;
-	GODateConventions const *date_conv;
-
-	Sheet		*target_sheet;
-	int              offset_col, offset_row;
-	gnm_float	 sum;
+	GnmCriteria *crit;
+	Sheet *target_sheet;
+	int offset_col, offset_row;
+	gnm_float sum;
 } SumIfClosure;
 
 static GnmValue *
 cb_sumif (GnmCellIter const *iter, SumIfClosure *res)
 {
-	GnmCell *cell;
-	if (NULL == (cell = iter->cell))
+	GnmCell *cell = iter->cell;
+	GnmValue *v;
+	if (!cell)
 		return NULL;
+
 	gnm_cell_eval (cell);
+	v = cell->value;
 
-	if ((VALUE_IS_NUMBER (cell->value) || VALUE_IS_STRING (cell->value)) &&
-	    (res->test) (cell->value, res->test_value, res->date_conv)) {
-		if (NULL != res->target_sheet) {
-			cell = sheet_cell_get (res->target_sheet,
-				iter->pp.eval.col + res->offset_col,
-				iter->pp.eval.row + res->offset_row);
-			if (cell != NULL) {
-				gnm_cell_eval (cell);
-				switch (cell->value->type) {
-				case VALUE_FLOAT:
-					/* FIXME: Check bools.  */
-					res->sum += value_get_as_float (cell->value);
-					break;
-				default:
-					break;
-				}
-			}
-		} else
-			/* FIXME: Check bools.  */
-			res->sum += value_get_as_float (cell->value);
+	if (!VALUE_IS_NUMBER (v) && !VALUE_IS_STRING (v))
+		return NULL;
+
+	if (!res->crit->fun (v, res->crit->x, res->crit->date_conv))
+		return NULL;
+
+	if (NULL != res->target_sheet) {
+		cell = sheet_cell_get (res->target_sheet,
+				       iter->pp.eval.col + res->offset_col,
+				       iter->pp.eval.row + res->offset_row);
+		if (!cell)
+			return NULL;
+
+		gnm_cell_eval (cell);
+		v = cell->value;
+
+		/* FIXME: Check bools.  */
+		if (!VALUE_IS_FLOAT (v))
+			return NULL;
 	}
 
+	/* FIXME: Check bools.  */
+	res->sum += value_get_as_float (v);
+
 	return NULL;
 }
 
@@ -715,7 +724,8 @@ gnumeric_sumif (GnmFuncEvalInfo *ei, GnmValue const * const *argv)
 	Sheet *start_sheet, *end_sheet;
 	SumIfClosure res;
 	GnmValue *problem;
-	CellIterFlags iter_flags;
+	GODateConventions const *date_conv =
+		workbook_date_conv (ei->pos->sheet->workbook);
 
 	/* XL has some limitations on @range that we currently emulate, but do
 	 * not need to.
@@ -732,8 +742,6 @@ gnumeric_sumif (GnmFuncEvalInfo *ei, GnmValue const * const *argv)
 	if (start_sheet != end_sheet)
 		return value_new_error_VALUE (ei->pos);
 
-	res.date_conv = workbook_date_conv (start_sheet->workbook);
-
 	if (argv[2]) {
 		GnmRange ra;
 		/* See 557782.  */
@@ -749,14 +757,13 @@ gnumeric_sumif (GnmFuncEvalInfo *ei, GnmValue const * const *argv)
 		res.target_sheet = NULL;
 
 	res.sum = 0.;
-	parse_criteria (argv[1], &res.test, &res.test_value, &iter_flags,
-			res.date_conv);
-#warning 2006/May/31  Why do we not filter non-existent as a flag, rather than checking for NULL in cb_sumif
+	res.crit = parse_criteria (argv[1], date_conv);
+#warning "2006/May/31  Why do we not filter non-existent as a flag, rather than checking for NULL in cb_sumif"
 	problem = sheet_foreach_cell_in_range
-		(start_sheet, iter_flags,
+		(start_sheet, res.crit->iter_flags,
 		 rs.start.col, rs.start.row, rs.end.col, rs.end.row,
 		 (CellIterFunc) &cb_sumif, &res);
-	value_release (res.test_value);
+	free_criteria (res.crit);
 
 	if (NULL != problem)
 	        return value_new_error_VALUE (ei->pos);
diff --git a/src/value.c b/src/value.c
index 2fd372d..49c3108 100644
--- a/src/value.c
+++ b/src/value.c
@@ -1532,6 +1532,13 @@ find_column_of_field (GnmEvalPos const *ep,
 	return column;
 }
 
+void
+free_criteria (GnmCriteria *criteria)
+{
+	value_release (criteria->x);
+	g_free (criteria);
+}
+
 /*
  * Frees the allocated memory.
  */
@@ -1541,16 +1548,9 @@ free_criterias (GSList *criterias)
         GSList *list = criterias;
 
         while (criterias != NULL) {
-		GSList *l;
 		GnmDBCriteria *criteria = criterias->data;
-
-		for (l = criteria->conditions; l; l = l->next) {
-			GnmCriteria *cond = l->data;
-			value_release (cond->x);
-			g_free (cond);
-		}
-
-		g_slist_free (criteria->conditions);
+		go_slist_free_custom (criteria->conditions,
+				      (GFreeFunc)free_criteria);
 		g_free (criteria);
 		criterias = criterias->next;
 	}
@@ -1560,59 +1560,64 @@ free_criterias (GSList *criterias)
 /**
  * parse_criteria :
  * @crit_val : #GnmValue
- * @fun : #GnmCriteriaFunc result
- * @test_value : #GnmValue the value to compare against.
- * @iter_flags :
  * @date_conv : #GODateConventions
  *
- * If @crit_val is a number set @text_val and @fun to test for equality, other
- * wise parse it as a string and see if it matches one of the available
- * operators.  Caller is responsible for freeing @test_value.
+ * Returns GnmCriteria which caller must free.
+ *
+ * ">=value"
+ * "<=value"
+ * "<>value"
+ * "<value"
+ * ">value"
+ * "=value"
+ * "pattern"
  **/
-void
-parse_criteria (GnmValue const *crit_val, GnmCriteriaFunc *fun, GnmValue **test_value,
-		CellIterFlags *iter_flags, GODateConventions const *date_conv)
+GnmCriteria *
+parse_criteria (GnmValue const *crit_val, GODateConventions const *date_conv)
 {
 	int len;
 	char const *criteria;
+	GnmCriteria *res = g_new0 (GnmCriteria, 1);
+
+	res->iter_flags = CELL_ITER_IGNORE_BLANK;
+	res->date_conv = date_conv;
 
-	if (iter_flags)
-		*iter_flags = CELL_ITER_IGNORE_BLANK;
 	if (VALUE_IS_NUMBER (crit_val)) {
-		*fun = criteria_test_equal;
-		*test_value = value_dup (crit_val);
-		return;
+		res->fun = criteria_test_equal;
+		res->x = value_dup (crit_val);
+		return res;
 	}
 
 	criteria = value_peek_string (crit_val);
         if (strncmp (criteria, "<=", 2) == 0) {
-		*fun = criteria_test_less_or_equal;
+		res->fun = criteria_test_less_or_equal;
 		len = 2;
 	} else if (strncmp (criteria, ">=", 2) == 0) {
-		*fun = criteria_test_greater_or_equal;
+		res->fun = criteria_test_greater_or_equal;
 		len = 2;
 	} else if (strncmp (criteria, "<>", 2) == 0) {
-		*fun = (GnmCriteriaFunc) criteria_test_unequal;
+		res->fun = (GnmCriteriaFunc) criteria_test_unequal;
 		len = 2;
-		if (iter_flags)
-			*iter_flags = CELL_ITER_ALL;
+		res->iter_flags = CELL_ITER_ALL;
 	} else if (*criteria == '<') {
-		*fun = (GnmCriteriaFunc) criteria_test_less;
+		res->fun = (GnmCriteriaFunc) criteria_test_less;
 		len = 1;
 	} else if (*criteria == '=') {
-		*fun = (GnmCriteriaFunc) criteria_test_equal;
+		res->fun = (GnmCriteriaFunc) criteria_test_equal;
 		len = 1;
 	} else if (*criteria == '>') {
-		*fun = (GnmCriteriaFunc) criteria_test_greater;
+		res->fun = (GnmCriteriaFunc) criteria_test_greater;
 		len = 1;
 	} else {
-		*fun = (GnmCriteriaFunc) criteria_test_equal;
+		res->fun = (GnmCriteriaFunc) criteria_test_equal;
 		len = 0;
 	}
 
-	*test_value = format_match (criteria + len, NULL, date_conv);
-	if (*test_value == NULL)
-		*test_value = value_new_string (criteria + len);
+	res->x = format_match (criteria + len, NULL, date_conv);
+	if (res->x == NULL)
+		res->x = value_new_string (criteria + len);
+
+	return res;
 }
 
 
@@ -1620,29 +1625,27 @@ static GSList *
 parse_criteria_range (Sheet *sheet, int b_col, int b_row, int e_col, int e_row,
 		      int   *field_ind)
 {
-	GnmDBCriteria *new_criteria;
-	GSList              *criterias = NULL;
-	GSList              *conditions;
-	GnmCell		    *cell;
-	GnmCriteria     *cond;
-	GODateConventions const *date_conv = workbook_date_conv (sheet->workbook);
-
+	GSList *criterias = NULL;
+	GODateConventions const *date_conv =
+		workbook_date_conv (sheet->workbook);
         int i, j;
 
 	for (i = b_row; i <= e_row; i++) {
-		new_criteria = g_new (GnmDBCriteria, 1);
-		conditions = NULL;
+		GnmDBCriteria *new_criteria = g_new (GnmDBCriteria, 1);
+		GSList *conditions = NULL;
 
 		for (j = b_col; j <= e_col; j++) {
-			cell = sheet_cell_get (sheet, j, i);
+			GnmCriteria *cond;
+			GnmCell	*cell = sheet_cell_get (sheet, j, i);
 			if (cell != NULL)
 				gnm_cell_eval (cell);
 			if (gnm_cell_is_empty (cell))
 				continue;
 
-			cond = g_new (GnmCriteria, 1);
-			parse_criteria (cell->value, &cond->fun, &cond->x, NULL, date_conv);
-			cond->column = (field_ind != NULL) ? field_ind[j - b_col] : j - b_col;
+			cond = parse_criteria (cell->value, date_conv);
+			cond->column = (field_ind != NULL)
+				? field_ind[j - b_col]
+				: j - b_col;
 			conditions = g_slist_prepend (conditions, cond);
 		}
 
diff --git a/src/value.h b/src/value.h
index fff0323..b570800 100644
--- a/src/value.h
+++ b/src/value.h
@@ -192,17 +192,18 @@ typedef struct {
         GnmCriteriaFunc fun;
         GnmValue  *x;
         int	   column; /* absolute */
+	CellIterFlags iter_flags;
+	GODateConventions const *date_conv;
 } GnmCriteria;
+
 typedef struct {
         int     row;	/* absolute */
         GSList *conditions;
 } GnmDBCriteria;
 
-void	parse_criteria		(GnmValue const *criteria,
-				 GnmCriteriaFunc *fun,
-				 GnmValue **test_value,
-				 CellIterFlags *iter_flags,
-				 GODateConventions const *date_conv);
+GnmCriteria *parse_criteria (GnmValue const *crit_val,
+			     GODateConventions const *date_conv);
+void	free_criteria		(GnmCriteria *criteria);
 void	free_criterias		(GSList *criterias);
 GSList *find_rows_that_match	(Sheet *sheet, int first_col,
 				 int first_row, int last_col, int last_row,