Re: [AD] Clarifying and revising filesystem API

[ Thread Index | Date Index | More lists.liballeg.org/allegro-developers Archives ]


On 2009-04-15, Thomas Fjellstrom <tfjellstrom@xxxxxxxxxx> wrote:
> On Wed April 15 2009, Elias Pschernig wrote:
> > Peter Wang wrote:
> > > * ALLEGRO_MAKE_TEMP_REMOVE_ON_OPEN
> >
> > That confuses me a bit... wouldn't it make more sense to say CLEAR_ON_OPEN?
> 
> Its actually removing the file. "clear" doesn't really do the operation 
> justice.
> 
> > > *(Q) As discussed, I think these should be removed*
> >
> > I still think so. Let's leave this to the physfs addon (and it will be up
> > to the addon author whether to hide physfs and provide wrappers for the
> > search path stuff, or have users call physfs functions and just provide
> > some glue).
> 
> Again, I disagree. I added it because ACTUAL PEOPLE wanted it. The fact that 
> it also supports a physfs operation directly, all the better.

Looking through physfs API made me more convinced that pushing these extra
knobs and settings to the backend would be better.  The write directory and
permitSymbolicLinks() functions were already mentioned.  We don't allow
control over whether to prepend or append a search directory.  PhysicsFS 2.0
even deprecates the search path functions by a more general mounting system.

The point is, each backend likely will have its own idea of how to do things,
and that might even change with newer versions.  Our abstraction is never
going to cut it, so we might as well define the barest essentials and tell
the user to use backend specific functions to tweak things.

> > > *(Q) Why not just use al_close_file_handle or al_destroy_file_handle?*
> > > *(Q) At least in the stdio version, this is no different to
> > > al_open_file_handle followed by a check that the path was a directory, so
> > > maybe this is not required?*
> >
> > There may be a reason - but right now, I also don't see it. If an
> > ALLEGRO_FILE_ENTRY is a directory, opening/closing it should always be the
> > same as al_open/close_dir. So those are indeed not needed.
> >
> > > *(Q) Peter Hull points out there is no rewind.*
> >
> > We could make al_fseek work with it. And independent of that an al_rewind()
> > which just does al_fseek(0) does not sound bad either.

> > > ## API: al_remove_file
> > >
> > > *(Q) Do we need this?*
> >
> > Why not. If my game removes a save-file, I probably have the location
> > available as a path... so this makes it easier than having to construct a
> > file handle first.
> 
> You might as well keep the handle around for it then no?

> > > ## API: al_update_file_handle_info
> > >
> > > *(Q) Does this need to be called before al_get_file_handle_mode, etc.?*
> >
> > I'd say no. Those functions should check a flag to see if stat() info is
> > available and if not call it. The only use for al_update_file_handle_info
> > would be if you suspect something might have changed since the last time
> > you called any of those functions (and therefore stat was called and the
> > info cached).
> 
> Indeed. And I don't think any of the functions should automatically update it. 
> stat is a rather heavy function. Should only update if and when the user says 
> they want to.

Ok.

> > > ## API: al_file_exists
> > > *(Q) I think this is unnecessary*
> >
> > We already have al_path_exists, so yeah. One question is, would those two
> > always have done the same?
> >
> > > ## API: al_file_is_regular
> > > *(Q) I think this is unnecessary*
> > > ## API: al_file_is_directory
> > > *(Q) I think this is unnecessary*
> > > ## API: al_get_file_mode
> > > *(Q) I think this is unnecessary*
> > > ## API: al_get_file_atime
> > > *(Q) I think this is unnecessary*
> > > ## API: al_get_file_ctime
> > > *(Q) I think this is unnecessary*
> > > ## API: al_get_file_mtime
> > > *(Q) I think this is unnecessary*
> > > ## API: al_get_file_size
> > > *(Q) I think this is unnecessary*
> >
> > Not sure, but they all seem uncommon enough that you could just as well
> > create a file handle and query the info using that.
> 
> Thats what the implementations do anyhow ;)

Ok.

> > > ## API: al_fclose
> > >
> > > *(Q) Why not just make this a synonym for al_destroy_file_handle?*
> >
> > Or remove it completely and rename al_fopen to al_open_file.
> 
> They do different things... al_fopen and al_fclose a slightly different 
> wrappers to open_handle and close_handle. The former two could possibly be 
> removed, but the later two are complex enough that no, you don't want to just 
> remove them.

> > > ## API: al_feof
> > >
> > > Returns true if we have an end of file condition.
> >
> > I'd like a note here to explicitly define the behavior when fungetc was
> > used.

Ok.  Still need to decide what the behaviour should be.

> > > ## API: al_fread_32le
> > >
> > > Reads a 32-bit word in little-endian format (LSB first).
> > >
> > > Returns:
> > > The read 32-bit word or EOF on error.
> >
> > I think this should be:
> >
> > """
> > Returns the read 32-bit word. If an error occurs the return value is
> > undefined. """
> >
> > The reason is, we apparently can't return anything here which isn't also a
> > valid return value. So we should not tempt the user to check for EOF or 0
> > or whatever before assuming an error (and consequently forget to do proper
> > error checking).

I forgot about that.  I am likely to forget it again when using it, so what
about:

    int32_t al_fread_32le(ALLEGRO_FS_ENTRY *entry, bool *ret_succeed);

> > > ## API: al_fwrite_32le
> > >
> > > Writes a 32-bit word in little-endian format (LSB first).
> > >
> > > Returns:
> > > The written 32-bit word or EOF on error.
> >
> > I would prefer this to either return void or the number of bytes written
> > (which would be either 4, or 0 on error).

You're right, same problem as before.

I think it should, following the example of al_fwrite, return the number of
bytes written on error (less than 4, not necessarily zero).

> > > ## API: al_fseek
> > > ## API: al_ftell
> >
> > We could make those work with directories (in the stdio driver, use
> > "seekdir" and "telldir" from libc). Or if not, we probably should state
> > that it's an error to call them on non-file entries.

My first reaction wasn't favourable, but I can't think of a reason not to
overload al_fseek/al_ftell for directories.

Peter





Mail converted by MHonArc 2.6.19+ http://listengine.tuxfamily.org/