Re: turning g_assert* into warnings

[Federico Mena Quintero <federico ximian com>]

On Fri, 2007-10-12 at 11:52 +0200, Tim Janik wrote:

> i'd like to propose to turn g_assert and friends like g_assert_not_reached
> into warnings instead of errors. i'll give a bit of background before the
> details though.
> 
> the main reasons we use g_return_if_fail massively throughout the glib and
> gtk+ code base is that it catches API misuses verbosely and gives programmers
> a chance to fix their code. this could be achieved with either a g_warning
> (mourn on stderr) or a g_error (mourn and abort).

g_return_*if_fail() is a filter:  "I don't want crap to come into my
library".  As you said, it's intended for programmers to fix their code
when they use libraries.

g_assert(), on the other hand, is a hard pre/post-condition. It is "if
this is false, then my program is fucked up".  There is *nothing* you
can do... user data may already be corrupt or you will likely screw it
up soon enough.  It's better to crash right there.

The idea is that you code libraries like this:

int
find_index_of_thingy (DataStructure *foo, Thingy thingy)
{
    int i;
    Bar bar;

    g_return_val_if_fail (foo != NULL, -1);

    i = 0;

    bar = foo->bar;
    while (bar) {
        g_assert (bar->eek != NULL);

        if (are_the_same (thingy == bar->eek))
             return i;

        i++;
        bar = next_bar (bar);
    }
}

I.e. if the public function gets called with bogus data, you return
immediately and print a warning, so that the programmer will fix his
code.  It will probably crash anyway if the messy programmer didn't
check the return value.

On the other hand, the assertion is an internal sanity check within the
library.  If the data structure contains a NULL bar->eek, it *REALLY*
means that something got fucked up earlier, and it's better to crash
right there, because you don't know *WHAT* is corrupt.

> c) programs that aren't 100% bug free could possibly trigger these warnings
>     during production. aborting would take all the end user data with it,
>     created/modified images, text documents, etc.
>     issuing just a warnig preserves the possibility to still save crucial
>     data if the application logic state still permits (which is most often
>     the case in practice).

This is just a symptom of an unrobust program.  You can't make a program
more robust by ignoring error cases, which is what your proposal is
about.  You may make it more *resilient* temporarily, but by then who
knows if you'll be saving bogus data.

Programs which deal with critical data (I'd say any kind of document is
critical) should really just implement auto-save.  If the bug is in
libsavemydata.so, well, life is hard...

> for a few reasons:
> 1) allow users to save their data if still possible; i.e. (c) from above.

So... which program crashed on you, losing your unsaved data, which
caused you to start this discussion? :)

> 2) for glib/gtk+ unit tests (more on that later), failing but
>     non-aborting tests are interesting for overall report generation; and the
>     ability to force immediate aborts for debugging is preserved with
>     --g-fatal-warninngs.

*shrug*.  A crashed test means "the test suite failed" :)

It may be mildly interesting to know "the test suite failed because
test_123() would have crashed", but why would you be releasing with test
suites that don't pass, anyway?

> 3) assertions can help to document programmer intentions and frequent (but
>     regulated) use as we have it with g_return_if_fail can significantly
>     reduce debugging time. i have so far been fairly strict about not adding
>     assertions to the glib/gtk+ core though, because they also tend to make
>     the code and end-user experiences more brittle, especially an
>     occasional wrong assertions that'd have to be revoked upon closer
>     inspection.

A wrong assertion is just a bug like any other.  It means "uuh, I wasn't
sure of the state my program may be in at this point, so I'll assert()
that it has the state I think it should have, and I'll fix it if it
crashes later".  It just indicates a sloppy programmer who should have
used a test suite with better code coverage :)

I've been guilty of wrong assertions in the past, and the description
above is *exactly* what happened to me --- laziness to *really* figure
out the implications of the code flow.

> as a side note, it'd probably make sense to present the end users with more
> prominent warnings if one of his applications ran into a glib warning/
> critical/assertion, and he should be notified about needing to save
> his data and exit ASAP. but that's really another topic, probably
> best tackled by gnome.

That's actually a nice idea, and would help get problems like "fooapp
spews thousands of the same g_warning() and .xsession-errors fills up"
fixed more quickly.

  Federico