[vlc-devel] [PATCH] Sqlite Module for VLC, GSoC 09

[Srikanth Raju]

Hi,

On Tue, Sep 15, 2009 at 11:58 PM, Laurent Aimar <fenrir at via.ecp.fr> wrote:
> Hi,
> On Tue, Sep 15, 2009, Srikanth Raju wrote:
>> >  I am not sure the 'transaction' part need to be in the function name. But if
>> > you prefer to have it then I think it would be better to use _ to be consistent
>> > with pf_query_callback and other vlc functions.
>> Removed the transaction keyword.
>> >> +
>> >> +    config_PutPsz( p_sql, "database-path", psz_host );
>> >> +    config_PutPsz( p_sql, "database-user", psz_user );
>> >> +    config_PutPsz( p_sql, "database-pass", psz_pass );
>> >> +    config_PutInt( p_sql, "database-port", i_port );
>> >  Not acceptable, it need to be moved into sql_t.
>> Done.
>
>  Thanks,
>
>  I think it would be better to split this patch in two:
>  - one that add sql_t object/functions
>  - one that add sqlite implementation of sql_t.
> as the first one does not depend on the second one.
> (I have not looked at the sqlite part)
>

Done!

>> +#define sql_Create( a, b, c, d, e, f ) __sql_Create( VLC_OBJECT(a), b, c, d, e, f )
>> +VLC_EXPORT( sql_t*, __sql_Create, ( vlc_object_t *p_this, char *psz_name,
>> +            char* psz_host, int i_port, char* psz_user, char* psz_pass ) );
>  Changing __sql_Create into sql_Create and moving the #define below would
> avoid adding more functions starting with __. (A #undef in misc/sql.c will
> also be needed).
>

I have done this. But I'm not sure if it's what you expected. Do
review it when possible.


>> +    }
>> +
>> +    vlc_object_set_destructor( p_sql, __sql_Destructor );
>  I am not sure that using vlc_object_set_destructor instead of a dedicated
> function to destroy a sql_t object is good.
>  At least with a dedicated function, it is easier to track when an object is
> created and destroyed.
>  Besides, a sql_t object will not be shared and so I don't think the refcounting
> here is a good thing, it might give bad ideas to people.
>  I would like to hear the opinions of other people on this one.
>

I don't see why there's a problem with sql_t being shared. Many modules might
share the same sql connection.

All the other issues are fixed.

-- 
Regards,
Srikanth Raju
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Sql-Core.patch
Type: text/x-diff
Size: 13092 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090916/3ba833d4/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Sqlite-Module.patch
Type: text/x-diff
Size: 16157 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090916/3ba833d4/attachment-0001.patch>