[cairo] How soon to call cairo_error?

[Carl Worth]

In typing up notes for the 1.5.2 snapshot, (really, I haven't
forgotten about making it---and I'm almost done), I took the chance to
read over the commit messages since 1.4.10.

Here's a commit that I apparently didn't notice when it first went in
among so many of Chris Wilson's obviously correct fixes to error
paths:

    commit bed8239f03773ad1584c8ba48ceb0b34bbe69453
    Author: Chris Wilson <chris at chris-wilson.co.uk>
    Date:   Thu Oct 4 13:15:46 2007 +0100

    [cairo-error] Clean up all the warnings and missing _cairo_error()
    calls.

    Every time we assign or return a hard-coded error status wrap that
    value
    with a call to _cairo_error(). So the idiom becomes:
        status = _cairo_error (CAIRO_STATUS_NO_MEMORY);
    or
        return _cairo_error (CAIRO_STATUS_INVALID_DASH);

    This ensures that a breakpoint placed on _cairo_error() will
    trigger
    immediately cairo detects the error.

I have three concerns when looking at this commit:

1) Cairo itself might handle the non-success status, so the
   lowest-level assignment might not actually be an error at all, (so
   reporting it with _cairo_error is just plain wrong).

2) We previously had tried to call _cairo_error as high as possible
   within cairo's stack, (as close to the user code as possible), so
   this change may introduce multiple calls to _cairo_error for a
   single error. That could be distracting.

3) The deeper calls to _cairo_error mean that the user will be plunged
   deeper into cairo's guts when debugging[*]. That could be annoying.

I did see at least one instance of case (1) get fixed up later:

    commit 7ff80234e3823547395819f96d7f7673df9ce9df
    Author: Chris Wilson <chris at chris-wilson.co.uk>
    Date:   Tue Oct 16 10:37:45 2007 +0100

    [cairo-path-fixed] Drop the _cairo_error() markup.

    Do not use _cairo_error(CAIRO_STATUS_NO_CURRENT_POINT) within
    _cairo_path_fixed_get_current_point() as the only caller,
    cairo_get_current_point(), expects and handles that status.

If we are extremely confident that there aren't more, then that's
fine. (By the way, Chris, how exhaustive has your error-checking work
been? Do you have coverage reports to show us? Or in general, have you
documented the strategy that has led to the phenomenal number of
patches you've made for cairo?)

The other two points are cosmetic to some extent. It really would make
it easier for users to debug their own code if given as little of
cairo's internals as possible. On the other hand, if we're missing any
calls to _cairo_error on any error-path then that's a fatal blow to
this debugging technique. I'm not confident that we weren't missing
any before this patch, so I'm more than willing to accept the
less-kind behavior as a cost of making sure we don't miss any.

But if there is any way to be confident that we're hitting all error
paths, (some sparse magic or something?), I would prefer to have the
_cairo_error calls as high up as possible.

So no real objection to the change here. Just some food for thought.

-Carl

[*] Currently this debugging requires the user to be aware of
_cairo_error and use a debugger to set a breakpoint on
this. Meanwhile, I'd much rather see a mode where _cairo_error prints
a stack trace of the user's code. How close are we to being able to do
that?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20071026/9be6f0be/attachment.pgp