[cairo] cairo_status_to_string and why I don't like CAIRO_OK

[Vladimir Vukicevic]

Out in Mozilla-land*, we have error codes constructed according to a
pattern that encodes failure, module ID, and the actual status code in
a 32 bit integer.  Failulre is indicated by the high bit being set, so
there are two macros, NS_FAILED(status) and NS_SUCCEEDED(status) that
are used to test return codes.  This type of scheme may be useful for
cairo as well, to allow for informative status codes to be returned
alongside failures and to clarify which component actually set the
error status once propagated upwards (for status values that are
unique and not shared amongst components).

Encoding success or failure that way can also simplify the
CAIRO_RETURN_* macros you suggest, by providing for
CAIRO_RETURN_IF_SUCCESS() and CAIRO_RETURN_IF_FAILURE(), though if
(CAIRO_SUCCEEDED(status)) return value; may be more readable (on two
separate lines).

     - Vlad

* See http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsError.h for
the full deal.

On 6/2/05, Carl Worth <cworth at redhat.com> wrote:
> Oh, I had promised "why I don't like CAIRO_OK" earlier, but I ended up
> discarding my original long-winded description when I found I disliked
> the original proposal I had intended to make.
> 
> But now that I've mentioned it, I guess I'll share my current
> thoughts. I've already renamed CAIRO_OK to STATUS_OK which is a more
> accurate name, though not as namespace-friendly.
> 
> The other thing I've never liked about CAIRO_OK is that it introduces
> a synonym of "OK" for "SUCCESS", (and the name CAIRO_OK has always
> sounded to me like it should be doing something like
> CAIRO_CHECK_SANITY does).
> 
> If we were to fix both the namespacing and the synonym then we'd be at
> something like:
> 
>         if (CAIRO_STATUS_SUCCESS (status))
> 
> which, incidentally, would clash with and provide no benefit[*] over
> directly using the enum that this macro is hiding:
> 
>         if (status == CAIRO_STATUS_SUCCESS)
> 
> [*] OK, ok. The first version is _one_ _character_ shorter.
> 
> Anyway, I think this little thought exercise has convinced me that
> CAIRO_OK isn't helping much. I'm inclined to make the following two
> changes:
> 
> Testing for an error in status:
>         if (! CAIRO_OK (status))        -> if (status)
> 
> Testing for success in status:
>         if (CAIRO_OK (status))          -> if (status == CAIRO_STATUS_SUCCESS)
> 
> The first condition could be written as (status != CAIRO_STATUS_SUCCESS)
> but I think the shorter form is sufficiently clear, (particularly
> since testing status in this sense is far more common and is the
> recommended sense to use in general).
> 
> One of the original intentions of CAIRO_OK was to replace uses of "if
> (!status)" when testing for success. I agree that that version is
> abhorrent and shall not appear in the implementation. I think that "if
> (status == CAIRO_STATUS_SUCCESS)" makes a fine replacement for it.
> 
> -Carl
> 
> PS. While on the subject, I also want to change CAIRO_CHECK_SANITY. I
> think I'll combine it with the nearly universal:
> 
>         if (cr->status)
>            return;
> 
> which is at the entry to all void cairo context functions, (or should
> be, I notice cairo_get_current_point is missing it).
> 
> Then, I'd like to drop the check from the end of each function, (since
> the next cairo function call will catch any problem).
> 
> We'll need consistent handling for the value-returning functions as
> well, (which already aren't checking cr->status as they should). So
> maybe the new macros we want are:
> 
>         CAIRO_RETURN_IF_STATUS (status)
> and:
>         CAIRO_RETURN_IF_STATUS_VALUE (status, return_value)
> 
> or something like that. These would have the assert statement for a
> valid status value in them. And the remainder of the implementation
> could perhaps benefit from similar macros without the assert:
> 
>         CAIRO_RETURN_IF (condition)
>         CAIRO_RETURN_IF_VALUE (condition, return_value)
>