[cairo] API Shakeup: cairo_begin_group, cairo_end_group, cairo_get_group

[Carl Worth]

On Thu, 07 Jul 2005 15:09:04 -0400, Owen Taylor wrote:
> On Thu, 2005-07-07 at 08:54 -0700, Carl Worth wrote:
>
>  2. Bitfields don't language-bind very well.. even using the C API
>     from C++ will produce compiler warnings without the addition
>     of casts.
> 
>     So, introducing one into Cairo for just this case perhaps isn't
>     a good idea.
> 
> I'd probably just go with a 3-element enum. (If you wanted to preserve
> the ability to have bitfields in the future, you could make the
> three values 0x1, 0x2, 0x3...)

Fair enough. I've eliminated the bitfield and added a third value
CAIRO_CONTENT_COLOR_ALPHA instead.

> +    /* XXX: If we switch to never returning a NULL surface, (like we
> +     * do for cairo_t and cairo_pattern_t), then we'd have the
> +     * advantage here of actually being able to indicate to the caller
> +     * what the error is here, rather than just returning a vague
> +     * NULL. */
> +
> +    /* Check that the content value is valid. */
> +    if (content == 0 ||        content & ~(CAIRO_CONTENT_COLOR |
> CAIRO_CONTENT_ALPHA))
> +       return NULL;
> ===
> 
> I don't think it's ever interesting for a program to check "if I passed
> a junk cairo_content_t, then I should...".

No, of course that wouldn't be useful. The point of my comment is that
the NULL pointer is going to cause a crash down the road somewhere and
at that point there won't be information available to the poor
person doing the debugging as to _why_ the pointer is NULL.

>                                            What is needed here (and
> elsewhere) is simply debug spew.

I likely do need to overcome my feeling that library code should never
print.

But, even if we had this, I'd still put the above comment in the
code. I would still want a way to indicate the connection between the
failing check here and the subsequent problems down the road. NULL
just doesn't carry that information.

NULL does have the advantage of forcing the user to notice something,
and likely notice sooner than a non-NULL result would here. The
non-NULL result, on the other hand, provides recoverability. And even
though narrow recovery doesn't make sense here, as you pointed out
above, I think that the ability to do broad recovery still improves
the robustness of cairo and makes it easier to use. Being able to do
something along the scale of "render an SVG image" and being able to
just check for success at the end and recover in the case of failure
can be quite useful.

Interestingly enough, I just had the chance to explore debugging based
on this exact error case. My previous patch was ridiculously broken
and was only getting correct results due to a remarkable
coincidence. I hadn't updated any of the calls to create_similar to
actually pass a cairo_content_t instead of a cairo_format_t, (14 calls
in the implementation and 4 in the test). It just so happened that:

	CAIRO_FORMAT_RGB24 == CAIRO_CONTENT_COLOR == 1
	CAIRO_FORMAT_A8 == CAIRO_CONTENT_ALPHA == 2

and even though CAIRO_FORMAT_ARGB32 == 0 while (CAIRO_CONTENT_COLOR |
CAIRO_CONTENT_ALPHA) == 3, I happened to have a default case that
mapped content==0 -> CAIRO_FORMAT_ARGB32.

When I switched from a bit-field to a zero-based enum, all the
CAIRO_CONTENT values shifted by one and things got interesting. So,
we've now got a problem in that cairo_content_t and cairo_format_t are
easy to confuse and thanks to C's lousy enum semantics, we've got no
type-safety to help the poor programmer keep things straight. I'm
tempted to replace the enums with some nasty *-swallowing pointer
typedefs to give us a type-safe interface that feels just like an enum
to use. I'll give this a try and see if I can make a header file
that's not too hideous.

Anyway, the debugging I just went through reminded me that what I
really want to be able to do is to put a breakpoint on _cairo_error
and have it stop there for any error that cairo detects. (Currently,
only cairo_t error are funneled through there).

After we've got that, I think all we'll need is an environment
variable that makes _cairo_error print the name of the calling
function and the status string. The ability to be noisy here will
definitely help in letting users communicate errors to coders that
don't have debugger access to misbehaving code.

> The 'format' argument is used here... your patch is going to break win32
> compilation.

Oops. You caught me in one of my lazy, let-the-compiler-tell-me-what-else-
I-need-to-do-to-finish-this-patch moments. Fixed.

> I don't think gtk-doc likes this sort of intermingling. And the output
> certainly won't preserve the intermingling. The enumeration constants
> should be the first thing in the doc comment, just like function 
> parameters.

I did wonder if I was cheating there. Thanks for the careful eye, as
usual. Also fixed.

> Other than that, the patch looks good.

Good. I'll come up with something for the type safety problem and then
post a new version.

-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/20050707/a9fcc686/attachment.pgp