After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 664072 - Folks should only use assert*() for critical, program-terminating errors
Folks should only use assert*() for critical, program-terminating errors
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal major
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2011-11-14 21:40 UTC by Travis Reitter
Modified: 2012-08-16 23:04 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Change some assert()s into return[_val]_if_fail()s (4.95 KB, patch)
2012-07-30 10:07 UTC, Philip Withnall
committed Details | Review

Description Travis Reitter 2011-11-14 21:40:51 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.
Comment 1 Guillaume Desmottes 2011-11-16 09:33:02 UTC
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().
Comment 2 Travis Reitter 2011-11-16 18:57:14 UTC
(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.
Comment 3 Philip Withnall 2012-07-30 10:07:15 UTC
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.
Comment 4 Travis Reitter 2012-08-16 20:51:02 UTC
Review of attachment 219874 [details] [review]:

Looks good to me.
Comment 5 Philip Withnall 2012-08-16 23:04:01 UTC
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(-)