[cairo] On allowing NULL font options

[Behdad Esfahbod]

On Fri, 2008-01-18 at 07:57 -0800, Carl Worth wrote: 
> 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?

Given that libraries shouldn't crash, yes.


> Detecting NULL and treating it as an error is obviously a good thing.

Right.  Even if crashing makes bug fixing easier!


> 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?

Well, that's a design problem with cairo_font_options_t.  The more
common annoyance with it is when you want to copy font options from one
object to another.  You end up with:

  cairo_font_options_t *font_options;

  font_options = cairo_font_options_create ();

  cairo_surface_get_font_options (some_surface, font_options);
  cairo_set_font_options (cr, font_options);

  cairo_font_options_destroy (font_options);

One object creation and two copies...


> 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.

Yes, that particular API is not interesting at all from an ease-of-use
point of view.  And one really very rarely wants to create a scaled font
using that api but with default font options.


> * 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.

Indeed.


> * 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.

Yes.  We already have healthy objects, in-error objects, and nil
objects.  The two latter ones are not distinguishable from the user's
point of view (except that one doesn't leak).  User just need to
condition on cairo_*_status() at any time to differentiate them.  Adding
NULL adds yet another class of objects.

Well, technically speaking, it's kinda useful yet unfortunate that we
documented all constructors as never returning NULL.  We didn't have to.
We could just say "whatever returned by this constructor can be passed
to other functions using one of these objects with no fear of bad
behavior because of errors".  We could then for example actually use
NULL as the magic value for OOM.  But we opted for never using NULL,
leaving that particular value useful for users to be able to tell
between "I've not created my cairo object yet" without an additional
state needed.


> 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?

Error and return.  Aborts may be easy for debugging, but definitely lose
user's work.  We should avoid that at all expenses in libraries.


> 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.

Right.  We talked about this previously.  Lemme recap my current state
of mind quickly.

Errors are of different kind:

  - Programming errors.  Like passing NULL where one shouldn't.  While
we should try to be lenient about these and definitely not crash and
control damage, these are the most interesting ones for developers.
These should be very easy to spot, by way of printing out messages for
example, even by default.  Bonus for having a mode to make these fatal
(for use during development).

  - Unavoidable errors.  Out-of-memory or a full disk fit into this
category.  Some are rare (out-of-memory), others not (full disk).  Some
decide to ignore and crash on the rarer ones.  As a library, would be
nice to handle them gracefully.  And we do a good job at this already.

  - Avoidable errors.  Like passing a degenerate matrix to a function
where one's not allowed.  We can say "don't do that", but that's too
heavy on the programmer to follow.  Do we really expect any matrix
coming in from an SVG or Flash file to be checked for invertibility
before being passed to cairo?  No.  Currently we deal with these the
same we as we do with unavoidable errors.  Why should save();
scale(0,0); restore() leave you in an error state?  One would argue that
because the drawings after the degenerate scale were erroneous.  Sure.
But what if there was no drawing?  What if the drawing was quite
well-defined with a 0,0 scale?  We should push the errors down to where
they actually happen.  And then allow recovery.  get_current_point() is
a great example of that.  We handle it gracefully: if there's no current
point, we don't set cairo_t into error state in the get_current_point()
call, but we don't allow recovery: we don't return a cairo_status saying
that there's no current point.


So, to summarize, we need to:

  - Make programming errors easy to spot, and be cool with them: check
input at all entry points and put into err state and print out something
more verbosely than other errors (say, as debug level 1)

  - Make avoidable errors really avoidable in an easy-to-use way.

Well, that's all for now.  No concrete actions.  Already too long.  Hope
that helps :).


> Chris, what do you think of all this?
> 
> -Carl
-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759