[cairo] New API proposal: cairo_restore_status()

[Andrea Canciani]

On Tue, Jan 11, 2011 at 12:33 PM, Andrea Canciani <ranma42 at gmail.com> wrote:
> A discussion on IRC pointed out the usefulness of some way to reset
> the error status of a cairo_t.
>
> Allowing to reset it directly is not likely to be a good idea, because its
> state might be invalid or inconsistent. To avoid this issue, it is possible
> to only allow resetting the status along with a valid state.
>
> The attached patches provide an implementation of cairo_restore_status():

Forgot the attachments in the previous mail.

>
> + * cairo_restore_status:
> + * @cr: a #cairo_t
> + *
> + * Restores @cr to the state saved by a preceding call to cairo_save()
> + * and removes that state from the stack of saved states. The status
> + * of @cr will be reset to the same value it was in before calling
> + * cairo_save().
> + **/
> +void
> +cairo_restore_status (cairo_t *cr)
>
> The implementation idea and an use case come from Benjamin Otte, but
> it has already been suggested in the ML multiple times.
>
> The rationale for not providing a way to reset the error status
> "[...] is that we don't have any
> guarantee that we can restore the context to any particular state
> after an error occurs. Without a huge audit and a lot of pain to make
> every cairo operation atomic, the only conceivable state to restore to
> would be the default state. And if we had an operation that did
> recover-error-and-return-context-to-default-state then I would argue
> that that wouldn't achieve anything that cairo_destroy;cairo_create
> does not also achieve, but that it could lead to confusion as people
> might not expect all of the state to be reset when recovering from
> errors."
> (from http://lists.freedesktop.org/archives/cairo/2006-September/007892.html
> )
>
> It appears that implementing such a feature is not as hard and
> painful as it was foreseen.
>
> The main idea is that *conceptually* the status is moved from the cairo_t to
> the state and cairo_state() always pushes a state, even if it cannot
> allocate
> memory. In this model, cairo_restore_status() just removes the top state
> (thus it restores the status of the previous state), while cairo_restore()
> removes the top state, but also copies the status from it to the previous
> state.
>
> This cannot cause any observable difference in existing applications,
> because cairo_save() and cairo_restore() keep exactly their existing
> semantic. It should not even cause bad interactions with libraries
> which decide to use cairo_restore_state(), as long as the bracketing
> save/restore is preserved correctly.
> It might cause some changes in the behavior of applications which
> call cairo_save() and then use a library function which was at first
> supposed to call cairo_restore() and then was changed to use
> cairo_restore_status().
>
> Examples:
> OK:
> main()
> {
> cairo_save();
> foo();
> cairo_restore(); /* this can be changed to cairo_restore_status() safely */
> }
>
> BROKEN:
> lib_func_which_calls_cairo_restore()
> {
> change_the_current_state();
> cairo_restore(); /* this CANNOT be changed to cairo_restore_status() safely
> */
> }
>
> main()
> {
> cairo_save();
> lib_func_which_calls_cairo_restore();
> }
>
> WORKAROUND:
> lib_func_which_calls_cairo_restore()
> {
> /* Adding a pair of internal brackets, solves the observability problem */
> cairo_save();
> change_the_current_state();
> cairo_restore_status();
> cairo_restore(); /* this CANNOT be changed to cairo_restore_status() safely
> */
> }
>
> main() {
> cairo_save();
> lib_func_which_calls_cairo_restore();
> }
>
>
> Details about why the implementation is correct (without the huge audit)
> follow:
>
>  - a count of virtual failure states is kept in the cairo_t. cairo_save() on
> an
>   cr with an invalid status just increments the number of virtual
> stack elements.
>   cairo_restore() on a cr with virtual stack elements just decrements this
>   number. This allows counting the stack elements without performing
>   allocation, so it is always successful.
>
>  - cairo_save/restore*() never modify a cr with an "excessive" number of
>   states in the stack. This allows a very clean workaround to operations
>   on nil cairo_t's and solves the problems that might arise if the counter
>   for virtual states wrapped around.
>
>  - real operations (not just counter incs/decs) are only performed on cr's
>   whose failure states count is 0
>
> ASSUMPTION: _cairo_gstate_save() can only fail because it cannot
>   allocate memory for the new state and in this case it will not modify
>   the current stack in any way.
>
>  - cairo_save() will either manage to create a new state and push it on
>   the top of the stack (resulting in a stack of valid states) or it will not
>   have enough memory to perform the allocation. In the latter case it
>   will push a virtual invalid state (i.e. set the failure count to 1 and the
>   status to an error state)
>
> ASSUMPTION: _cairo_gstate_restore() can only fail if the stack is just
>   the current state.
>
>  - cairo_restore() will simply restore the previous state, polluting it with
>   the current status (if it is an error)
> NB: the new implementation reflects this: cairo_restore() now removes
>   the top state even if the cr is in an error status
>
>  - cairo_restore_status() will restore the previous state, including the
>   status. If the failure states counter is 0, the status of the previous
>   state was obviously CAIRO_STATUS_SUCCESS.
>
>  - cairo_restore*() always set the cr to an invalid status if the pop
>   operation on the stack is not legitimate
>
> Would this be an acceptable addition for 1.12?
> Should (some of) this documentation be added as comments to the
> actual code? Would adding the assumptions (and/or asserting them)
> be sufficient?
>
> Andrea
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-cairo_restore_status.patch
Type: application/octet-stream
Size: 4225 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20110111/f9e70e0d/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-test-Add-save-restore.patch
Type: application/octet-stream
Size: 4346 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20110111/f9e70e0d/attachment-0001.obj>