[cairo] On allowing NULL font options

[Behdad Esfahbod]

Ping?  What are we going to do with this for 1.6?

On Sat, 2008-01-19 at 22:27 -0500, Behdad Esfahbod wrote:
> 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