Re: turning g_assert* into warnings

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

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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]