Re: [AD] file interface fopen

[Peter Wang <novalazy@xxxxxxxxxx>]

On 2010-12-20, Matthew Leverton <meffer@xxxxxxxxxx> wrote:
> On Mon, Dec 20, 2010 at 2:07 AM, Peter Wang <novalazy@xxxxxxxxxx> wrote:
> > al_fopen_vt perhaps has a purpose.  I can't see one for
> > `al_create_file_handle(void *userdata)'.  A concern is that you might
> > accidentally pair the default interface with an unrelated userdata.
> >
> I don't think it's particularly useful either.
> 
> Attached is a new patch (against 5.1 svn). I finished it before I saw
> the above comment, so it includes al_create_file_handle_vt(). It can
> easily be renamed and moved with al_create_file_handle().
> 
> Otherwise, I think it's as dicussed.

Some nitpicks below.

> Index: include/allegro5/file.h
> ===================================================================
> --- include/allegro5/file.h	(revision 14136)
> +++ include/allegro5/file.h	(working copy)
> @@ -19,7 +19,7 @@
>   */
>  typedef struct ALLEGRO_FILE_INTERFACE
>  {
> -   AL_METHOD(ALLEGRO_FILE*, fi_fopen, (const char *path, const char *mode));
> +   AL_METHOD(void *,  fi_fopen, (const char *path, const char *mode));
>     AL_METHOD(void,    fi_fclose, (ALLEGRO_FILE *handle));
>     AL_METHOD(size_t,  fi_fread, (ALLEGRO_FILE *f, void *ptr, size_t size));
>     AL_METHOD(size_t,  fi_fwrite, (ALLEGRO_FILE *f, const void *ptr, size_t size));
> @@ -37,6 +37,7 @@
>  struct ALLEGRO_FILE
>  {
>     const ALLEGRO_FILE_INTERFACE *vtable;
> +   void *userdata;
>  };

ALLEGRO_FILE can now be moved into aintern_file.h

> Index: src/file.c
> ===================================================================
> --- src/file.c	(revision 14136)
> +++ src/file.c	(working copy)
> @@ -2,23 +2,73 @@
>  #include "allegro5/internal/aintern.h"
>  #include "allegro5/internal/aintern_file.h"
>  
> +#include <stdio.h>

I didn't see any need for this.

>  /* Function: al_fopen
>   */
>  ALLEGRO_FILE *al_fopen(const char *path, const char *mode)
>  {
> -   const ALLEGRO_FILE_INTERFACE *drv = al_get_new_file_interface();
> +   return al_fopen_vt(al_get_new_file_interface(), path, mode);
> +}
> +
> +
> +/* Function: al_fopen_vt
> + */
> +ALLEGRO_FILE *al_fopen_vt(const ALLEGRO_FILE_INTERFACE *drv, const char *path, const char *mode)
> +{ 
> +   ALLEGRO_FILE *f =  NULL;
> +   
> +   ASSERT(drv);  
>     ASSERT(path);
>     ASSERT(mode);
> +   
> +   if (drv->fi_fopen) {
> +      f = al_malloc(sizeof(*f));
> +      if (!f) {
> +         al_set_errno(ENOMEM);
> +      }
> +      else {
> +   	     f->vtable = drv;

Stray tabs here and elsewhere.

> +         f->userdata = drv->fi_fopen(path, mode);
> +         if (!f->userdata) {
> +            al_free(f);
> +            f = NULL;
> +         }
> +      }
> +   }
> +   
> +   return f;
> +}

> Index: docs/src/refman/file.txt
> ===================================================================
> --- docs/src/refman/file.txt	(revision 14136)
> +++ docs/src/refman/file.txt	(working copy)
> @@ -16,7 +16,7 @@
>  
>  The fields are:
>  
> -    ALLEGRO_FILE* (*fi_fopen)(const char *path, const char *mode);
> +    void*         (*fi_fopen)(const char *path, const char *mode);
>      void          (*fi_fclose)(ALLEGRO_FILE *handle);
>      size_t        (*fi_fread)(ALLEGRO_FILE *f, void *ptr, size_t size);
>      size_t        (*fi_fwrite)(ALLEGRO_FILE *f, const void *ptr, size_t size);

You should document how fi_fopen works now.

> @@ -391,3 +397,24 @@
>  
>  See also: [al_store_state], [al_restore_state].
>  
> +### API: al_create_file_handle
> +
> +Creates an empty, opened file handle with some abstract user data.
> +This allows custom interfaces to extend the [ALLEGRO_FILE] struct
> +with their own data. After it is finished, it should be destroyed
> +by the standard [al_fclose] function.
> +
> +See also: [al_fopen], [al_fclose], [al_set_new_file_interface]
> +
> +### API: al_create_file_handle_vt
> +
> +Creates an empty, opened file handle using the specified interface.
> +
> +See also: [al_create_file_handle]
> +
> +### API: al_get_file_userdata
> +
> +Returns a pointer to the custom userdata that is attached to the
> +file handle. This is intended to be used by functions of extended
> +[ALLEGRO_FILE_INTERFACE]s.

Reword the sentence to avoid writing "[ALLEGRO_FILE_INTERFACE]s".
That will come out badly in some output formats.

> Index: addons/physfs/a5_physfs.c
> ===================================================================
> --- addons/physfs/a5_physfs.c	(revision 14136)
> +++ addons/physfs/a5_physfs.c	(working copy)
> @@ -15,7 +15,6 @@
>  
>  struct ALLEGRO_FILE_PHYSFS
>  {
> -   ALLEGRO_FILE file;      /* must be first */
>     PHYSFS_file *phys;
>     int pushback;           /* -1 if none */
>     bool error_indicator;
> @@ -24,13 +23,18 @@
>  /* forward declaration */
>  static const ALLEGRO_FILE_INTERFACE file_phys_vtable;
>  
> +const ALLEGRO_FILE_INTERFACE *_al_get_phys_vtable()
> +{
> +   return &file_phys_vtable;
> +}

Write (void) for no argument functions (I saw the prototype is already so).

You forgot to free the ALLEGRO_FILE handle in al_fclose.

Peter