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
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@gnome.org
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
