Re: [PATCH] tests/unit/test-char.c: Fix error handling issues

[Peter Maydell <peter.maydell@linaro.org>]

On Wed, 9 Jun 2021 at 13:36, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Tue, 8 Jun 2021 at 20:51, Marc-André Lureau
> > <marcandre.lureau@redhat.com> wrote:
> >>
> >> Hi
> >>
> >> On Tue, Jun 8, 2021 at 9:06 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>> I think that improving the quality of the failure reporting
> >>> in 'make check' is useful, and that we should probably turn
> >>> on g_test_set_nonfatal_assertions() everywhere. (The worst that
> >>> can happen is that instead of crashing on the assert we proceed
> >>> and crash a bit later, I think.) Awkwardly we don't have a single
> >>> place where we could put that call, so I guess it's a coccinelle
> >>> script to add it to every test's main() function.
> >>>
> >>
> >> I don't have any strong opinion on this. But I don't see much sense in
> >> having extra code for things that should never happen.
> >
> > The point is that I want to make them happen, though...
>
> I'd prefer not to.
>
> Writing tests is tedious enough as it is.  Replacing
>
>     assert COND in one of the many ways GLib provides
>
> by
>
>     assert COND in one of the many ways GLib provides
>     if (!COND) {
>         bail out
>     }
>
> makes it worse.
>
> Readability suffers, too.

I agree. But glib doesn't provide a "check this test thing I'm
trying to test, and make it cleanly abandon and fail the test
if the check passes" function. I suppose we could rig one up
with setjmp/longjmp and some macros...

> >> I would teach coverity instead that those asserts are always fatal.
> >
> > If you want an assert that's always fatal, that's g_assert().
> > These ones are documented as not always fatal.
>
> You'd sacrifice the additional output from g_assert_cmpint() & friends,
> which can sometimes save a trip through the debugger.  I don't care all
> that much myself, but I know others do.

> Plain assert()'s behavior is configurable at compile time: assertion
> checking on / off.  This sets a trap for the unwary: side effects in the
> argument.  We avoid the trap by gluing the compile-time switch to "on".
>
> GLib's optionally non-fatal assertions add new traps, with much less
> excuse.  Without recovery code, non-fatal assertions make little sense.
> But when you have to add recovery code anyway, you could easily switch
> to a new set of check functions, too.  Overloading the existing
> assertion functions was in bad taste.

I agree that I wouldn't have named them _assert myself...

-- PMM