Re: custom model rows-reordered marshal

[Torsten Schoenfeld <kaffeetisch gmx de>]

Kevin Ryde wrote:
> Torsten Schoenfeld <kaffeetisch gmx de> writes:
>> So I applied your patch.
> 
> I suspect it's still not quite right, I've got doubts about the ST() in
> the ->get_value() supplied-columns case, the same as was wrong with what
> I first tried for g_object_get.

Oh, jeez.  You're right.  Patch with failing test case and a fix attached.  Does
this look good to you?

I wonder how many stack handling bugs like this still hide in our PPCODE sections.

-Torsten

Index: t/GtkTreeModelIface.t
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/t/GtkTreeModelIface.t,v
retrieving revision 1.10
diff -u -d -p -r1.10 GtkTreeModelIface.t
--- t/GtkTreeModelIface.t	31 Aug 2008 16:46:08 -0000	1.10
+++ t/GtkTreeModelIface.t	6 Sep 2008 12:23:32 -0000
@@ -346,7 +346,7 @@ sub sort {
 
 package main;
 
-use Gtk2::TestHelper tests => 177, noinit => 1;
+use Gtk2::TestHelper tests => 178, noinit => 1;
 use strict;
 use warnings;
 
@@ -508,7 +508,11 @@ use warnings;
 
 $model = StackTestModel->new;
 is_deeply ([ $model->get ($model->get_iter_first) ],
-           \ StackTestModel::ROW,
+           [ @StackTestModel::ROW ],
            '$model->get ($iter) does not result in stack corruption');
 
+is_deeply ([ $model->get ($model->get_iter_first, reverse 0 .. 9) ],
+           [ reverse @StackTestModel::ROW ],
+           '$model->get ($iter, @columns) does not result in stack corruption');
+
 # vim: set syntax=perl :
Index: xs/GtkTreeModel.xs
===================================================================
RCS file: /cvsroot/gtk2-perl/gtk2-perl-xs/Gtk2/xs/GtkTreeModel.xs,v
retrieving revision 1.59
diff -u -d -p -r1.59 GtkTreeModel.xs
--- xs/GtkTreeModel.xs	31 Aug 2008 16:46:08 -0000	1.59
+++ xs/GtkTreeModel.xs	6 Sep 2008 12:23:32 -0000
@@ -1130,35 +1130,50 @@ gtk_tree_model_get (tree_model, iter, ..
 	get_value = 1
     PREINIT:
 	int i;
-    PPCODE:
+    CODE:
+	/* we use CODE: instead of PPCODE: so we can handle the stack
+	 * ourselves. */
 	PERL_UNUSED_VAR (ix);
-	/* The code below uses PUTBACK and SPAGAIN to synchronize the local
-	 * with the global stack pointer.  This is done so that other xsubs
-	 * (invoked indirectly by gtk_tree_model_get_value) don't overwrite
-	 * what we put on the stack. */
-	if (items > 2) {
+#define OFFSET 2
+	if (items > OFFSET) {
 		/* if column id's were passed, just return those columns */
-		for (i = 2 ; i < items ; i++) {
+
+		/* the stack is big enough already due to the input arguments,
+		 * so we don't need to extend it.  nor do we need to care about
+		 * xsubs called by gtk_tree_model_get_value overwriting the
+		 * stuff we put on the stack. */
+		for (i = OFFSET ; i < items ; i++) {
 			GValue gvalue = {0, };
-			PUTBACK;
 			gtk_tree_model_get_value (tree_model, iter,
 			                          SvIV (ST (i)), &gvalue);
-			SPAGAIN;
-			XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+			ST (i - OFFSET) = sv_2mortal (gperl_sv_from_value (&gvalue));
 			g_value_unset (&gvalue);
 		}
-	} else {
+		XSRETURN (items - OFFSET);
+	}
+#undef OFFSET
+
+	else {
 		/* otherwise return all of the columns */
+
 		int n_columns = gtk_tree_model_get_n_columns (tree_model);
+		/* extend the stack so it can handle 'n_columns' items in
+		 * total.  the stack already contains 'items' elements so make
+		 * room for 'n_clumns - items' more, move our local stack
+		 * pointer forward to the new end, and update the global stack
+		 * pointer.  this way, xsubs called by gtk_tree_model_get_value
+		 * don't overwrite what we put on the stack. */
+		EXTEND (SP, n_columns - items);
+		SP += n_columns - items;
+		PUTBACK;
 		for (i = 0; i < n_columns; i++) {
 			GValue gvalue = {0, };
-			PUTBACK;
 			gtk_tree_model_get_value (tree_model, iter,
 			                          i, &gvalue);
-			SPAGAIN;
-			XPUSHs (sv_2mortal (gperl_sv_from_value (&gvalue)));
+			ST (i) = sv_2mortal (gperl_sv_from_value (&gvalue));
 			g_value_unset (&gvalue);
 		}
+		XSRETURN (n_columns);
 	}
 
  ## va_list means nothing to a perl developer, it's a c-specific thing.