[cairo] [cairo-commit] 2 commits - src/cairo.c src/cairo-font-face.c src/cairo-font-options.c src/cairo-ft-font.c src/cairo-gstate.c src/cairo-scaled-font.c src/cairo-win32-font.c test/font-options.c test/.gitignore test/Makefile.am

[Carl Worth]

On Thu, 17 Jan 2008 14:39:27 -0800 (PST), Chris Wilson wrote:
>     [cairo-font-options] Treat NULL as a default cairo_font_options_t
>
>     Interpret a NULL cairo_font_options_t as the default values - i.e
>     as if it were a fresh pointer returned by
>     cairo_font_options_create().

I like this idea. Thanks for the work!

> +    if (other != NULL) {
> +	options->antialias = other->antialias;
> +	options->subpixel_order = other->subpixel_order;
> +	options->hint_style = other->hint_style;
> +	options->hint_metrics = other->hint_metrics;
> +    } else
> +	_cairo_font_options_init_default (options);
>  }

A very minor point, (which I only mention since I had more worthwhile
things to comment on the same time), I would have done:

	if (other == NULL)
	   special_handling ();
	else
	   standard_handling ();

That's both to take care of the exceptional first, (as discussed
somewhat in CODING_STYLE), and also because I think conditions
without negation are easier to read.

>  cairo_font_options_equal (const cairo_font_options_t *options,
>  			  const cairo_font_options_t *other)
>  {
> +    if (options == NULL)
> +	options = &_cairo_font_options_nil;
> +    if (other == NULL)
> +	other = &_cairo_font_options_nil;
> +
> +    if (options == other)
> +	return TRUE;

This part made me a tiny bit uncomfortable at first. First, I didn't
like the idea of treating a user's NULL (meaning "defaults") the same
as a nil (meaning "an error occurred"). Similarly, for most of the nil
objects, we initialize them with a bunch of meaningless values,
expecting them to never be examined---so that kind of thing would
break down here, (where the subsequent code compares the actual
values).

My second concern is moot because this particular nil object happens
to be initialized carefully to default values:

	static const cairo_font_options_t _cairo_font_options_nil = {
	    CAIRO_ANTIALIAS_DEFAULT,
	    CAIRO_SUBPIXEL_ORDER_DEFAULT,
	    CAIRO_HINT_STYLE_DEFAULT,
	    CAIRO_HINT_METRICS_DEFAULT
	};

But that's still at least theoretically fragile, (that is, it would be
less fragile if NULL would be supported by using something that at
least went through _cairo_font_options_init_default). I'd also be less
nervous about the fragility if we were using a name like
_cairo_font_options_default instead of _cairo_font_options_nil for
this object.

My first concern is still valid though. We don't store an explicit
status value in a cairo_font_options_t but we implicitly return
CAIRO_STATUS_NO_MEMORY for the nil object. So is it legitimate that
cairo_font_options_equal (NULL, nil) returns TRUE while
cairo_font_options_status (NULL) returns SUCCESS, but
cairo_font_options_status (nil) returns NO_MEMORY?

I don't know that that is very likely to cause any actual problems in
applications, but it does seem odd at least.

So adding a _cairo_font_options_default next to
_cairo_font_options_nil would address all my (admittedly weak)
concerns. But I'm sure Behdad won't like the .data implications. (Or
is it free in this case given the other object already exists?)

-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.cairographics.org/archives/cairo/attachments/20080117/1c7af158/attachment.pgp