Re: turning g_assert* into warnings

["Brian J. Tarricone" <bjt23 cornell edu>]

Tim Janik wrote:
> hey All.
>
> 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.

Like Mathias, I was in a bit of "hell no!" mode when I first read this. 
 After reading your rationale, I'm less opposed, but... well, still 
opposed.

> 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).
>
> the reasons for the default behavior to use g_warning:
> a) a warning usually fullfills its purpose, the programmer knows there's
>     something to fix and can start to trace the root cause.
> b) for easy tracing in gdb with backtraces, programmers can use
>     --g-fatal-warnings to force abort behavior.
> 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 reasonable, and pretty much covers why I use g_return_if_fail() 
and friends.

> in a recent discussion, i figured that (c) perfectly applies to g_assert
> and g_assert_if_reached() also.

Sorry, but I just don't buy it.  When I use g_assert(), my thinking is: 
"this is something that's a pretty bad bug on my part, and if the 
assertion fails, I'm almost 100% sure that the program is going to crash 
very very soon, and possibly in unpredictable ways."

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

If I think it's possible for users to still do something meaningful with 
the program, I'll use a g_return_if_fail() or a custom if(foo) { 
g_warning(); do_something(); } type thing. That's why we have the 
separate macros in the first place!  g_return*() are for cases where the 
programmer thinks recovery might be possible, and g_assert*() is where 
it isn't possible.

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

And that's the right approach, IMO: use assertions sparingly.  Sometimes 
one might be a mistake, and you fix it.

>     conditionalizing the abort removes the brittleness and allows for
>     adding more helpful assertion statements.

No, it doesn't.  Changing a g_assert*() to a g_return*() when you 
realise aborting is unnecessary is the way to go.

> note that in practice, this shouldn't change anything for programmers
> (except for the ability to write better code ;)
> because of G_DISABLE_ASSERT, programmers can already not rely on
> failing assertions to abort their programs (only g_error will reliably
> do that).

... which I think is pretty broken.  You shouldn't be able to disable 
asserts and pre-condition checks (G_DISABLE_CHECKS) at compile time. 
They were put there for a reason.

> it should however take down programs in end user scenarios less often,
> and code review can be more lax about adding assertions.

No -- instructing programmers to make *proper* use of g_assert*() (and 
to use g_return*() otherwise) will bring programs down less often.  Or 
maybe not -- maybe everyone uses it "properly" and this problem you just 
doesn't exist.

Also...  The documentation tells us what g_return*() and g_assert*() do. 
 If you change this, you're essentially changing the API of glib. 
That's... not cool.  You can argue that G_DISABLE_CHECKS and 
G_DISABLE_ASSERT will do this anyway, but that's a choice on the person 
who builds the code (and again, I'd argue that the existence of those 
defines is a *really* bad idea).  IMO, if you build with 
G_DISABLE_(CHECKS|ASSERTS), you get to keep the pieces when things break.

	-brian