GNOME Bugzilla – Bug 664072
Folks should only use assert*() for critical, program-terminating errors
Last modified: 2012-08-16 23:04:18 UTC
We have some GLib.assert() and assert_not_reached() calls throughout our code. Since we don't build our code with -DG_DISABLE_ASSERT, these asserts end up terminating running code. This is particularly bad since Gnome Shell now depends upon us; we shouldn't ever exit early but instead work around errors like bug #663889 with a warning. In many cases, like above, we may need to change our code. If there are any cases where the code can safely continue, we should retain the assert*() and build with -DG_DISABLE_ASSERT.
AFAIK most modules don't do that. An assertion is supposed to be critical enough to exit if it fails. If you just want to raise a warning use g_warning() and g_return_if_fail() rather than g_assert().
(In reply to comment #1) > AFAIK most modules don't do that. An assertion is supposed to be critical > enough to exit if it fails. > > If you just want to raise a warning use g_warning() and g_return_if_fail() > rather than g_assert(). Yeah, like I said above, we should just fix the way we use assert() for the cases where it's too extreme. I'll fix the title.
Created attachment 219874 [details] [review] Change some assert()s into return[_val]_if_fail()s https://www.gitorious.org/folks/folks/commits/664072-die-assertions-die That should be all of them which we want to change. The rough reasoning I used was: • If an assert() depends on control flow (inside libfolks) more than data flow, then it remains as an assert(). • If an assert() depends mainly on user-inputted data, then it turns into a return[_val]_if_fail(). • Exception: if an assert() is in the IndividualAggregator, it depends on both control and data flow, but since we can’t guarantee a sane state after such an assertion failure, it’s safer that we use assertions in the IA rather than return[_val]_if_fail()s.
Review of attachment 219874 [details] [review]: Looks good to me.
Comment on attachment 219874 [details] [review] Change some assert()s into return[_val]_if_fail()s commit 4ae4e6398af89b341f0b32da74cb6f91b9fb5176 Author: Philip Withnall <philip@tecnocode.co.uk> Date: Mon Jul 30 12:03:21 2012 +0200 Bug 664072 — Folks should only use assert*() for critical, program-terminati Turn various assert()s into return[_val]_if_fail()s. Closes: https://bugzilla.gnome.org/show_bug.cgi?id=664072 NEWS | 2 ++ backends/telepathy/lib/tpf-persona-store.vala | 3 ++- folks/individual.vala | 14 +++++++------- folks/potential-match.vala | 8 ++++---- 4 files changed, 15 insertions(+), 12 deletions(-)