[cairo] Error handling patch

[Owen Taylor]

On Wed, 2005-07-27 at 15:01 -0700, Carl Worth wrote:

> > * In several places, you have:
> > 
> >   if (scaled_font->status)
> >     _cairo_scaled_font_set_error (scaled_font, scaled_font->status);
> > 
> >   [ same for cairo_pattern, etc. ]
> > 
> >   Is this really useful compared to just returning? If we make
> >   _cairo_error() debug spew, then we'll get tons and tons of irrelevant
> >   debug spew after the first root cause error.
> 
> It seemed useful to me. Suppose we start returning "default" values
> for the getters discussed above. I can imagine being inside a debugger
> and being confused by bogus values getting returned from cairo
> functions while not seeing the _cairo_error breakpoint getting
> hit. WONTFIX?

I think we have to get people into the habit of finding the *first*
_cairo_error, one way or the other; trying to debug further down
is going to be a waste of time. I don't really expect people to
run the code halfway and *then* set a breakpoint into _cairo_error()
in any case.

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.

[...]

> Another answer may very well be that create_in_error() was just a bad
> idea. The goal was to preserve the error value when it propagated from
> one object to another. But now that we have everything going
> consistently through _cairo_error, we could probably live with that
> information loss on error propagation, just return
> &_cairo_surface_nil, and maintain the convention that a failing create
> function never requires a destroy.

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

And not try to guess whether a particular create can produce
an object that needs to be freed. (There are some other places than
create_similar() in your patch that do it - cairo_glitz_surface_create()
for example.)

To me, having the actual error code in the returned object is of minimal
use [it's most useful for language bindings, which will be turning
statuses into exceptions], so B), which keeps the error code paths
simpler seems like a better choice.

However, we then really need to make *sure* that we don't create errored
objects from create() .. it's fairly easy to have a create function that
calls some other function that then sets an error.

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.)

> > * Your change to _cairo_surface_allocate_clip_serial() makes it
> >   violate its docs ("will not return 0") 
> 
> It it sufficient to just document that an in-error surface will return
> 0? If so, this is FIXED.

Not sure. keithp would be in a better position to answer this. My
worry is that you end up with a surface in an inconsistent state -
the cairo_surface_t has a clip_serial of zero, but a clip is set
on the underlying surface.

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.

Regards,
						Owen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050727/301e087b/attachment.pgp