[cairo] On allowing NULL font options

[Carl Worth]

On Thu, 17 Jan 2008 20:17:58 -0500, Behdad Esfahbod wrote:
> On Thu, 2008-01-17 at 17:39 -0500, 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 thought about this before.  I think we shouldn't doing it.  Currently
> the cairo practice is: no NULL.

This one was my fault.

The bug report that came in was a segfault due to a NULL
cairo_font_options_t* being passed to cairo_scaled_font_create.

Chris first fixed this by fixing cairo_font_options_t to return
CAIRO_STATUS_NULL_POINTER for NULL which is obviously correct, right?
Detecting NULL and treating it as an error is obviously a good thing.

Then, I stepped in and imagined some buggy code doing:

	cairo_scaled_font_t *scaled_font;

	scaled_font = cairo_scaled_font_create (font_face,
						matrix,
						ctm,
						NULL);

and imagining what the fix would have to be:

	cairo_scaled_font_t *scaled_font;
+	cairo_font_options_t *font_options;

+	font_options = cairo_font_options_create ();
	scaled_font = cairo_scaled_font_create (font_face,
						matrix,
						ctm,
						font_options);
+	cairo_font_options_destroy (font_options);

And this seemed a nuisance to me from an easy-to-use
point-of-view. Why make the user create something just to immediately
throw it away, but never putting any information into it?

But I think I was wrong for several reasons:

    * This is not the toy API, and if you're setup to do
      cairo_scaled_font_create then you've already done enough nasty
      work to create your font_face that this little bit of extra work
      is not significant. That is, this isn't code that every new user
      of cairo is going to have to right, so ease-of-use is not as
      much a priority as it is in other parts of the API.

    * The second form of the above is vastly more readable. The bare
      NULL in the first example communicates nothing about what it is
      doing. It's just as bad as the bare TRUE and FALSE values that I
      dislike so much in other APIs. Compare that to the second
      example where the purpose of "font_options" is quite obvious,
      and it's also quite obvious where to look for more information
      if there are options that need to be changed.

    * Finally, the reason that was brought up later in this thread
      suggests that a bare NULL in the code was likely not the problem
      here in the first place, (as the original author would have
      found that quite quickly). Instead there's likely some non-cairo
      function that's returning a NULL cairo_font_options_t* and we
      definitely don't want to encourage that kind of thing.

So I take back my original suggestion, (and I apologize to Chris for
the extra work).

And that just leaves what to do with NULL. Do we treat this as an
error or just immediately abort?

Behdad, you said something about wanting to print an error message. I
definitely don't have any problem printing a descriptive error just
before aborting, (in fact I think cairo should never consciously
abort without printing a description). And I think that we should
allow people to opt-in (environment variable?) to a detailed print
(with a backtrace!) at every cairo error.

Chris, what do you think of all this?

-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/20080118/44597d5c/attachment.pgp