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

[Carl Worth]

On Thu, 09 Jun 2005 10:19:38 -0400, Owen Taylor wrote:
> I don't think we should write this. If we are frequently writing
> 
>  if (status) {
>  }
> 
> Then we should simply write
> 
>  if (!status) {
>  }
>
> They are both pretty unreadable until you learn the idiom. 

If both of these forms appear in the code, then there's no idiom to
learn. The code is just plain hard to read.

My take on this is that "if (status)" is shorthand for "if (status !=
CAIRO_STATUS_SUCCESS)", and the only way that idiom is learnable is
that there's only shorthand for the test in one sense, (which is also
overwhelmingly more common in the implementation).

> (t would read better if we were using 'error' rather than 'status', 

Yes, 'error' would solve the problem. And I almost proposed exactly
that earlier. The problem comes when dealing with the common case of
things actually working. What do we name the success value?
CAIRO_ERROR_NONE ? Somehow I find it less satisfying to say "this
function didn't fail" instead of "this function succeeded".

A similar problem shows up at the public interface. The cairo_status
function currently answers two question:

  * Has everything been successful?

  * If not, how did things fail?

The name "cairo_status" seems to capture both questions well. But to
me "cairo_error" only seems to apply well to the second question. That
could be fixed by adding an is-there-an-error predicate, but that
seems just more awkward to use with no other benefit.

I guess I'd like the interface to be more natural for the common case
of everything working just fine. And "error" breaks that.

> > 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.
> 
> I find that really hard to read and type.

Why don't we just rewrite that code to use "if (status)" and early
return on error?

I just checked, and currently there are only five cases of "if (status
== CAIRO_STATUS_SUCCESS)" in the code. They all look like this:

	status = perform_some_operation ();
	if (status == CAIRO_STATUS_SUCCESS)
	    follow_up_on_operation ();

	return status;

And they could all be replaced with:

	status = perform_some_operation ();
	if (status)
	    return status;

	follow_up_on_operation ();

	return status;

Admittedly, that's two lines longer. But, I think it's much more
readable.

It follows the early-bail-out-on-error style recommended in "Managing
nested blocks" at the end of cairo/CODING_STYLE. Granted, there's no
savings in pulling a big block out of nesting here due to the single
statement. But, more significantly, now only error-handling is
relegated to a nested level so it's easy to read the primary purpose
at the top level of the function.

Of course, what we really want to write is:

	perform_some_operation ();

	follow_up_on_operation ();

But we are using C so there's only so much we can do. (But stay tuned
for more on that in the forthcoming error-handling patches.)

-Carl
-------------- 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/20050609/d7471f37/attachment.pgp