It is possible for a scaled_font to be flagged as in error, though only
through a "true error" e.g. a malloc failure. However, when returning a font
from the cache it should not be in error. So if we find a error font in the
cache we remove it and return a new font.
Fixing this uncovered a leak of a CAIRO_INT_STATUS_UNSUPPORTED value
up to cairo_show_page, (and similarly to cairo_copy_page). There was
really no good reason for _cairo_surface_show_page and
_cairo_surface_copy_page to be returning cairo_int_status_t. Fix
this by simply handling the UNSUPPORTED return at the surface layer
instead of the gstate layer.
Obviously, the new name is _cairo_pdf_surface_emit_to_unicode_stream which
is consistent with the to_unicode_stream identifiers already existing in
the implementation.
This class of functions modify their argument and return it as a
*convenience* for the caller. For example, within cairo a common idiom is:
cairo_object_reference (new);
cairo_object_unreference (this->obj);
this->obj = new;
which updates the member to the new object irrespective of whether the
new object is the same as the one being replaced. Other issues arise
with subtypes, as the return type is the parent's and so require more
complicated handling to compile cleanly.
Disabling the warning is therefore preferred over adding code which
decreases readibility and reduces maintainability. We need to make the
compiler work for us, not against us...
As previously implemented, there's no essential information in the
return value from _cairo_dotostr, (the caller can simply use strlen
to recompute the same value, which is what the only caller is already
doing).
There would be real information in a return value which would return
the result from the call to snprintf for the case where the buffer is
not large enough for the number being printed.
Add a _cairo_traps_status function and use it instead of adding
error checks to callers of _cairo_traps_add_trap and
_cairo_traps_add_trap_from_points, (both of which are now given
a void return type).
Note: It looks like it would be convenient to shove a status value
into cairo_path_fixed_t to reduce this sequence of error checks.
But I tried that first, and it actually makes things worse overall
due to many things like _cairo_path_fixed_move_to called by
cairo_move_to where the result must be immediately checked anyway.
So I've already rejected that approach.
Now, the functions to add new data to a polygon all become void,
and there's a new _cairo_polygon_status call to query the status
at the end of a sequence of operations.
With this change, we fix many callerswhich were previously not
checking the return values of _cairo_polygon functions by adding
only a single call to _cairo_polygon_status rathern than several
new checks.
The cairo_image_surface_create function never returns NULL so
the previous error checks would never have triggered. The correct
approach is to check the result of cairo_surface_status().
This is just multiplication after all, so there's nothing that can fail.
And we can get rid of a lot of useless error-checking code this way.
The corrected functions are:
_cairo_gstate_user_to_device
_cairo_gstate_user_to_device_distance
_cairo_gstate_device_to_user
_cairo_gstate_device_to_user_distance
Now that we have the warn_unused_result attribute enabled, (thanks
Chris!), it's actually harmful to have a function return an
uncoditional value of CAIRO_STATUS_SUCCESS. The harm is that
it would force lots of unnecessary error-checking paths that
just add clutter.
It is much better to simply give a function that cannot fail
a return type of void.
A few tests were failing due to clip_init_deep_copy() not being able to
clone the target surface. Before propagating the failure, this was being
silently ignored.
Copy the simple implementation from cairo-image-surface.
Currently if the ownership of the bitmap->buffer is passed to
_get_bitmap_surface() then the status of the buffer is inconsistent
should the function detect an error (i.e. CAIRO_STATUS_NO_MEMORY).
Fix it up that should we encounter an error and we own the buffer then
we always free it on behalf of the caller.
Confusion had been introduced as to who provided the fixup after
the malloc failed which resulted in a NULL deference whilst checking for
an erroneous pattern in _cairo_pattern_create_in_error.
Currently the code defaults to setting its points to NULL and fixing it up
on the first add_point() to use the embedded buffer. Skip this extra step
by initialising points to the embedded buffer.
The idiom for cairo.c is to do
cr->status = _cairo_op ();
if (cr->status) _cairo_set_error (cr, cr->status);
Unfortunately a trivial mistake for a _cairo_op () is to call a cairo_op ()
and forget to check cr->status but return CAIRO_STATUS_SUCCESS which will
mask the earlier error.
Obviously this is a bug in the lower level but the impact can be reduced
by chaning cairo.c to use a local status variable for its return:
cairo_status_t status = _cairo_op ();
if (status) _cairo_set_error (cr, cr->status);
These were found during a cairo_static pass on an alternative branch...
A critical one in particular was setting the have added glyph flag to
TRUE even if _cairo_xlib_surface_add_glyph() fails. This can cause an
application crash due to a RenderBadGlyph error later when the scaled
font is cleaned and we attempt to remove the glyph.
_cairo_pattern_release_surface() asserts that it is passed a pattern
surface. This itself is bad as breaks the symmetry with
_cairo_pattern_acquire_surface under() error conditions, however reorder
the cleanup to avoid this assertion.
The text perf-case tries to fill the region with a single text string,
but fails to detect when the current point does not advance due to an
error. This causes the perf-case to enter an infinite loop, so we break
out when the cairo_status() has been set.
