Re: warning about invalid tree model iterators

[Tim Janik <timj imendio com>]

On Fri, 17 Feb 2006, Owen Taylor wrote:

> On Fri, 2006-02-17 at 11:01 -0600, Federico Mena Quintero wrote:
> > On Fri, 2006-02-17 at 14:29 +0100, Tim Janik wrote:
> >
> >
> > > to help the compiler catch these mistakes, i've prepared a patch that adds
> > > G_GNUC_WARN_UNUSED_RESULT to all relevant iterator functions, and intend
> > > to commit that next week unless objections pop up. in principle it does:
> >
> > This is pretty nice.  It may create a few warnings for code that knows
> > that certain operations will succeed, but better help the rest of the
> > world :)
>
> Can we see some examples of code that was broken and what it should be
> doing?

this sounds a bit like you're questioning the iter functions returning a
boolean rather than void...
but for the sake of the argument, the code portion that markku originally
was debugging and triggered his suggestion was written by kris:

      path = gtk_tree_model_filter_elt_get_path (filter_iter->user_data,
                                                filter_iter->user_data2,
                                            filter->priv->virtual_root);

      gtk_tree_model_get_iter (filter->priv->child_model, child_iter, path);
      gtk_tree_path_free (path);

under some circumstances, no iter could be retrived from path, leading to
triggering an assertion in gtk_tree_path_free() because child_iter->stamp==0.

another good example is gtk_tree_model_iter_children() which returns FALSE
for parent iters without children which is obviusly easily triggered by
leaf nodes.

> It sounds like you are saying that if I do:
>
> /* prepend a row at the beginning of the list */
>
> /* prepend another row to the beginning of the list */
>
> /* Do something with the returned iterator */
>
> /* Advance the iterator one row */
>
> [ Yes, this is obviously artificial, but I can't think of a real
>  reason for   calling iter_next() not in a loop at the moment ]
>
> You think I should write:
>
> if ((!gtk_tree_model_iter_next(model, iter)) {
>     /* do something */
> }
>
> But what "do something" could I possibly do? About all I can think
> of to insert in there is g_assert_not_reached();

usually it's the other way around, where you do:

  gtk_tree_model_get_iter_from_string (model, iter, string);
  /* or gtk_tree_model_iter_next (model, iter), etc. */

  { /* some code */ }

and due to tree updates, or because the model hasn't been populated yet,
you should actually be doing:

  if (gtk_tree_model_get_iter_from_string (model, iter, string))
    { /* some code */ }


maybe you start to feel stronger about always wanting to check the return
value once you've run into forgetting to check it and find the result hard
to debug. this has happened to me once or twice, and now i'm watching others
facing the same problem.

i admit that artificial cases where checking the return value is not
required are easily created, but the actual harm involved is just a
mild annoyance. that is in contrast to forgetting to check the return
value which in practice easily happens, and requires significant debugging
work (possibly involving multiple persons, triggering of specific code
paths, etc.).
that, in combination with the fact that in most cases you ought to
evaluate the return value of the tree_model_iter functions anyway, leads
me to believe that allowing the compiler to catch omissions early in the
development process is going to be the less painful way and produce
the more robust code.

as a side note, the very fact that you find it hard to think of an
example for a unchecked iter_next() call is an indication for bogus
warnings to be seldomly triggered.

> This seems a little similar to making gtk_widget_show() return
> a boolean value that you are supposed to check because you might
> have passed in a NULL pointer for the widget.

i don't quite see the parallel. while i've never missed a return value when
using gtk_widget_show(), with tree model iterators you will want to check
the return value in most every case, either because you're looping anyway,
or to produce reasonably robust code. this has at least been my experience,
and probably also is the reason that i didn't see any new warnings when
compiling gtk+ with the above patch applied.

> Regards,
> 						Owen

---
ciaoTJ