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.