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

[Carl Worth]

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)



-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20050602/9ae71b1a/attachment.pgp