[cairo] Error handling patch

[Carl Worth]

On Wed, 27 Jul 2005 18:40:48 -0400, Owen Taylor wrote:
> It's not a major issue, but my opinion would definitely be that
> we only need to call _cairo_error() when an object is put into an
> error state and not subsequently.

That sounds reasonable.

I think we're likely to learn the right style for _cairo_error as we
gain some experience actually debugging real problems.

I know people have requested error printing, so we may end up adding a
__FILE__ and __FUNCTION__ arguments to _cairo_error and put some
environment-variable-triggered prints in there.

> We should have one of two conventions:
>  
>  A) Always destroy the result of create, even on the failing code path
> 
>  B) Never destroy the result of create

I don't think it's feasible to sync up on either of those at this
moment. It would be a fair amount of work and likely a large patch
either direction. I'd prefer to get to (A) eventually as I think the
code will be cleaner.

The convention I think we can pull off right now is to make create
either (a) always require destroy or (b) never require destroy on
error.

And I think (b) is the right option. Though it should still always
_permit_ a call to destroy. That would give us semantics similar to
malloc/free.

So I'll go through and rip out all the create_in_error stuff.

> The other question is what we document - if we document that you should
> always free the result of create(), then we give ourselves flexibility,
> but do make life harder for callers. (Except that most callers won't
> check at all.)

I vote for documenting the public interface as I described above. That
is, that successful create requires destroy, failed create does not
require destroy, but will allow destroy just fine.

This seems to allow two different modes from user code, neither of
which is "hard":

1)
	surface = cairo_surface_create ();
	/* ... */
	cairo_surface_destroy (surface);

2)
	surface = cairo_surface_create ();
	if (cairo_surface_status (surface))
	    BAIL;
	/* ... */
	cairo_surface_destroy (surface);

Here (1) is preferred and is the whole point of this work. And I have
tried to audit things well enough so that this approach will work
already from the public interface. The only open question above is
whether we're ready to use this style throughout the
implementation. We can probably just move to it later/gradually.

> On the other hand, since _cairo_surface_set_clip_region() becomes
> a no-op on a surface with a status, it's probably perfectly
> harmless.

That was my take on it. Things do get a lot easier to analyze when
most functions turn into no-ops.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050727/28b01230/attachment.pgp