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...
>> Also, why not make it an enum?
>
> It was an enum originally, but cedric thought bit flags would be better.
BAD cedric ;-)
>> > +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
