Re: [ft] FT_Done_Face Should Not Return An Error

[Gregor Mückl]

On 5/5/2018 11:54 AM, Lawrence D'Oliveiro wrote:
> It is common for object-disposal routines to never return error
> statuses. The archetypal example is free(3)
> <https://linux.die.net/man/3/free>. If this is passed a valid pointer,
> it disposes of the object; if it is passed NULL, it quietly returns
> without doing anything. If it is passed an invalid pointer, then this
> indicates a program bug, so there is no point returning an error code
> anyway: better to report an error message to stderr and even abort the
> program.
>
> Looking at the code for FT_Done_Face, from src/base/ftobjs.c:
>
>        FT_EXPORT_DEF( FT_Error )
>        FT_Done_Face( FT_Face  face )
>        {
>          FT_Error     error;
>          FT_Driver    driver;
>          FT_Memory    memory;
>          FT_ListNode  node;
>
>
>          error = FT_ERR( Invalid_Face_Handle );
>          if ( face && face->driver )
>          {
>            face->internal->refcount--;
>            if ( face->internal->refcount > 0 )
>              error = FT_Err_Ok;
>            else
>            {
>              driver = face->driver;
>              memory = driver->root.memory;
>
>              /* find face in driver's list */
>              node = FT_List_Find( &driver->faces_list, face );
>              if ( node )
>              {
>                /* remove face object from the driver's list */
>                FT_List_Remove( &driver->faces_list, node );
>                FT_FREE( node );
>
>                /* now destroy the object proper */
>                destroy_face( memory, face, driver );
>                error = FT_Err_Ok;
>              }
>            }
>          }
>
>          return error;
>        }
>
> the only thing that can really go wrong is if the face cannot be
> found among the driver’s memory blocks. Clearly something has gone
> catastrophically wrong in this case, so it makes sense to report an
> error and abort the program.
>
> Otherwise, if it is passed NULL, it should just quietly return without
> doing anything. This makes it easier to write code that initializes all
> temporary pointers up front and unconditionally disposes them at the
> end; there is no need to tediously check everything for NULL pointers,
> because the disposal routines will take care of that.

Please don't create another library that blindly terminates its host 
program on a whim. Returning an error really is the right thing to do 
here. Just make it clear that this is a bad error.

Calling exit() uncontrollably on an error in a library is bad for a 
whole list of reasons, including but not limited to:

- robs host program of any chance to do a sane cleanup and graceful exit
- prevents the generation of an error report in programs that have this 
QA feature
- circumvents cleanup and error handling mechanisms of other programming 
languages (e.g. C++ stack unwinding)

Regards,
Gregor