On Wed, Jul 28, 2010 at 8:18 PM, Gustavo Sverzut Barbieri <
barbi...@profusion.mobi> wrote:
> On Wed, Jul 28, 2010 at 10:05 PM, Stephen Houston <unixti...@gmail.com>
> wrote:
> > On Wed, Jul 28, 2010 at 6:33 PM, Gustavo Sverzut Barbieri
> > <barbi...@profusion.mobi> wrote:
> >>
> >> On Wed, Jul 28, 2010 at 7:12 PM, Enlightenment SVN
> >> <no-re...@enlightenment.org> wrote:
> >> > Log:
> >> > Add the ability to perform standard IO operations on a file in a
> >> > thread.
> >> > +#define EIO_FILE_MOD_TIME 1
> >> > +#define EIO_FILE_SIZE 2
> >> > +#define EIO_FILE_EXISTS 4
> >> > +#define EIO_FILE_IS_DIR 8
> >> > +#define EIO_FILE_CAN_READ 16
> >> > +#define EIO_FILE_CAN_WRITE 32
> >> > +#define EIO_FILE_CAN_EXECUTE 64
> >>
> >> You should provide some EIO_FILE_STAT, to return the whole stat
> >> result, maybe that is more useful than doing it separated for size,
> >> exists... you can leave what you did, but also provide the generic
> >> function as people will consult more than one property at time.
> >
> > I left that out because you can specify as many of the operations as you
> > want anyway.
>
> You force people to define many callbacks, or one with a big switch
> that is called multiple times. Still you don't provide other stat
> information that may be useful, like owner, mode...
>
> Right. What I put in now just covers what was in ecore_file. With it
being the flag style, it is obviously intended to grow in capabilities :)
I think a full stat flag is reasonable and I will add it.
>
> >> Also, why not make it an enum?
> >
> > It was an enum originally, but cedric thought bit flags would be better.
>
> BAD cedric ;-)
>
I tend to agree with Cedric though. Enums are intended to be used as states
or single options. Bit flags are better for specifying multiple flags.
>
>
>
> >> > +static void
> >> > +_eio_file_op_end(void *data)
> >> > +{
> >> > + Eio_File_Op *async;
> >> > +
> >> > + async = data;
> >>
> >> Just do Eio_File_Op *async = data, it is smaller and makes sense as
> >> you're just converting the parameter.
> >>
> > Ahhh the E formatting... :(
>
> strictly it is not e formatting, raster uses what you did, but it just
> adds lines to code, and leads to easier "unused variable" errors for
> no good.
>
>
>
> >> > + if (!async->exists)
> >> > + {
> >> > + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
> >> > + async->main_cb((void *)EINA_FALSE, EIO_FILE_EXISTS, data);
> >> > + ecore_thread_cancel(async->common.thread);
> >> > + return;
> >> > + }
> >>
> >> I don't like this, you don't return anything for CAN_READ, CAN_WRITE
> >> and others in the case it does not exists? Not even an error? IMO
> >> the user should always get called back, with an error whenever
> >> appropriate. Or call error_cb if there were requirement not fulfilled.
> >>
> >> also a bug there: the last parameter is USER data, not the one you got
> >> from the callback ("async"), you really want: async->common.data
> >> instead!
> >>
> > Actually CAN_READ CAN_WRITE and CAN_EXECUTE will return error if the file
> > doesn't exist (see if (!async->exists)). Otherwise is EINA_TRUE or
> > EINA_FALSE as to whether it can do those. Good catch on the bug.
>
> I fail to see how they will return error, if the file did not exist,
> then it will report for EIO_FILE_EXISTS and return :-/
>
>
>
>
> >> > + if (async->flags & EIO_FILE_EXISTS == EIO_FILE_EXISTS)
> >> > + async->main_cb((void *)EINA_TRUE, EIO_FILE_EXISTS, data);
> >>
> >> this is ugly and took me a while to notice why you did the EINA_TRUE
> >> constant here... it was because of the other test before. And really,
> >> why not call this one first?
> >>
> > I'm not sure what you are getting at here? It returns FALSE if it doesn't
> > exists, and TRUE if it does. The reason the FALSE is called first is
> > because the app will seg if any other operations are done on a file that
> > doesn't exists.
>
> I did notice that, but it took some time as at first it looked like a
> bug "he should use the variable and not a constant!", then I noticed
> the other case was handled before.
>
> what I meant to call this one first, is that we should dispatch the
> EIO_FILE_EXISTS before dispatching CAN_READ, CAN_WRITE...
>
>
>
> --
> Gustavo Sverzut Barbieri
> http://profusion.mobi embedded systems
> --------------------------------------
> MSN: barbi...@gmail.com
> Skype: gsbarbieri
> Mobile: +55 (19) 9225-2202
>
------------------------------------------------------------------------------
The Palm PDK Hot Apps Program offers developers who use the
Plug-In Development Kit to bring their C/C++ apps to Palm for a share
of $1 Million in cash or HP Products. Visit us here for more details:
http://p.sf.net/sfu/dev2dev-palm
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
