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 probability that a node of level L is generated is
0.25^(L-1) * 0.75. It means, a node of level 15 or
more will be used with a probability of about 3 * 10^-9.
That's really rare...
Actually that's not still true, because the level of a new
node is capped by current max-level plus one. So to really
get a node with a level of 15 one should first get a node
of level 2, then 3, then 4, ..., finally 15. Now that's
REALLY rare.
And guess what, the skiplist only start behaving bad with a
max level cap of MAX_LEVEL when having on the order of
4**MAX_LEVEL items in it. I really hope we don't get there.
Most memory allocators allocate in multiples of twice the size of
a pointer. So there is no point in keeping freelists for both
even and odd levels. We now round odd levels up to the next
even level for freelist computations. This reduces the number of
node mallocations.
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.
Detect when a substitute image surface is returned for a solid pattern,
and avoid mixed image/xlib composite operations. This can happen for example
if there is a resource allocation failure during creating a similar surface.
Add status returns to functions in order to propagate an error up
the call stack.
For the emit_*_pattern we add a new status return even when when
the functon return CAIRO_STATUS_SUCCESS unconditionally in order for
the caller to handle all cases in a consistent manner.
This adds a compiler check that the function result is used by the caller
and enables it by default for all cairo_private functions and for public
API that returns a cairo_status_t.
It has been discussed that to extend the warnings to all functions, a
new function type could been introduced to cover static functions:
cairo_static. This has not been done at the present time in order to
minimise the churn and focus on the more common errors.
In order to reduce the warning spew generated by gcc for invalid use of
this attribute, -Wno-attributes is added to CFLAGS. This has the
unfortunate side-effect of masking future warnings for all attributes -
be warned!
