Re: warning about invalid tree model iterators

[Owen Taylor <otaylor redhat com>]

On Fri, 2006-02-17 at 19:52 +0100, Tim Janik wrote:
> 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...

Most of the tree model functions return boolean because you can
use them to iterate over collections of unknown length - where
that length can include 0. E.g., the classic:

 GtkTreeIter iter;

 gboolean valid = g_model_iter_children(model, &iter, &parent);
 while (valid) { 
     /* do something with &iter */

     valid = g_model_iter_next(&iter);
 }

> 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.

But isn't that a bug in whatever provided the path? What do you write in
the else clause of the gtk_tree_model_get_iter()? At the very least, in
order to write anything there other than g_assert_not_reached() you have
to know *why* the path corresponded to a not in the model.

> 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.

In the end I don't really care about this a whole lot ... as you say,
you are normally checking these returns anyways.

I just wanted to point out the connection to error handling in general.
In order to provide an error return at all, and certainly in order
to force the programmer to check it, two conditions must be met:

 A) The error must be triggered by conditions outside the programmer's
    control. (gtk_widget_show() should not return an error if the
    programmer passes in NULL.)

 B) There must be some sensible recovery that the programmer can
    do. (gtk_widget_show() should not return an error if a font file
    on disk can't be opened so the widget can't be shown.)

GTK+ shouldn't follow the road of some parts of the Java API's, 
where checked exceptions force you to write:

 try {
    reader = new InputStreamReader(instream, "UTF-8");
 } catch (UnsupportedCharsetException e) {
    throw new RuntimeException("WTF, UTF-8 isn't supported???", e);
 }

Anyways, I just wanted to throw a little note of caution in about
G_GNUC_WARN_UNUSED_RESULT... I don't really remember the TreeModel
interfaces well enough at this point to say whether it's 1% or 20%
of the time where you don't need to check the result.

Regards,
						Owen