[cairo] Error handling patch
Owen Taylor
otaylor at redhat.com
Wed Jul 27 15:40:48 PDT 2005
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
More information about the cairo
mailing list