[vlc-devel] [PATCH] file extension manager (fixed patch)

[ßєŋ]

On Thu, Feb 19, 2009 at 11:05 PM, Rémi Denis-Courmont <rem at videolan.org> wrote:
> Le jeudi 19 février 2009 04:27:58 ßєŋ, vous avez écrit :
>> This is an (hopefully) fixed version of my previous patch. Please
>> ignore the old one. Sorry for the mistakes.
>
> A bunch of comments...
>
> +/**
> + * Initialise extension manager.
> + *
> + * \retval  VLC_SUCCESS  on success
> + * \retval  VLC_EGENERIC on failure
> + */
>
> Not sure if Doxygen supports multiple \retval per function.
>

I'm sure it does.

> +LIBVLC_EXTERN int fileext_Init(void);
>
> LIBVLC_EXTERN is for... LibVLC public APIs. Internal VLC core APIs are
> simply "extern", which is usually not even specified since it is the default.
>
>

extern won't link against C++. ... As far as VLC core is in C I agree with you.


> +  /* XXX: dynamic file extension:
> +     returns MALLOCated strings caller should FREE them !
> +  */
> +# include <vlc_fileext.h>
> +# define _EXTENSIONS_AUDIO    fileext_Get( FILEEXT_AUDIO )
> +# define _EXTENSIONS_VIDEO    fileext_Get( FILEEXT_VIDEO )
> +# define _EXTENSIONS_PLAYLIST fileext_Get( FILEEXT_PLAYLIST )
> +# define _EXTENSIONS_MEDIA    fileext_Get( FILEEXT_MEDIA )
> +# define _EXTENSIONS_SUBTITLE fileext_Get( FILEEXT_SUBTITLE )
>
> Identifier with a leading underscore are reserved by the C standard.
> It is best to not add anymore of these than we already have.
>

My bad. I have modified these for searching previous use of theses macros.
I should have restored original names.

> diff --git a/modules/gui/qt4/dialogs_provider.hpp
> b/modules/gui/qt4/dialogs_provider.hpp
> index c08a6a5..391765d 100644
> --- a/modules/gui/qt4/dialogs_provider.hpp
> +++ b/modules/gui/qt4/dialogs_provider.hpp
>
> It would be nice to split this into multiple patches (new API and back-end,
> using the new API, and removal of old cruft).
>

Damn it. That's what I did first :)

> +   int len =
> +      strlen (psz_playlist) + strlen(psz_allfiles) + strlen(psz_pl_exts) +
> 16;
> +
> +   list = new char [len];
> +   if( list )
> +   {
> +      sprintf(list,"%s|%s|%s|*",psz_playlist,psz_pl_exts,psz_allfiles);
>
> Using asprintf() (or QString) would be simpler and less likely to break if
> someone changes the code later.
>

I don't know anything about Qstuff; never used and never wanted to.
asprintf is a convenient GNU extension.

> +fileext_TypeName
> +fileext_TypeOf
> +fileext_Get
> +fileext_Add
> +fileext_Del
>
> Did $(make check) allow this? I think libvlccore.sym is supposed to be sorted.
>
> +/* Copy buffer with lowercase transform */
> +static inline void lowercpy(char * d, const char * s, const int l)
> +{
> +   int i;
> +   for( i = 0; i < l; ++i )
> +      d[i] = tolower(s[i]);
> +}
>
> Use size_t for array sizes. (I am also not a big fan of const for by-value
> parameters, as they're kinda useless, but I don't think we have a policy
> against that).
>

Totally useless in that context. I'm copying exclusively file
extension that are never longer than
a few chars. size_t looks like  a waste too me.

> +/* MUST BE LOCK
>
> s/LOCK/LOCKED/
>
> +      fileext = realloc(pool->fileext, newmax * sizeof(fileext_t));
>
> Potential integer overflow? Maybe not really an issue.
>

In what world ? 16 bit integer ? 65536/12 = 5461 extensions before a
possible overflow :D

> +void fileext_Cleanup(void)
> +{
> +   pool_cleanup(&pool);
> +}
> +
> +/**
> + * Initialise extension manager.
> + *
> + * \retval  VLC_SUCCESS  on success
> + * \retval !VLC_SUCCESS  error code of failure
> + */
> +int fileext_Init(void)
> +{
> +   int err;
> +
> +   err = pool_init(&pool) != VLC_SUCCESS;
>
> With the public API, we may have multiple instances in the same process. I am
> afraid this is not thread-safe.
>

Process or thread ? That's all the difference. Processes do not share
data so each have it's own pool. At init time (and cleanup) I have
assume there is only one thread. This init should take place before
module loading. I thought the source location where I have included
the calls to theses functions was right. I may be wrong as I'm not
well aware of VLC general architecture.

> +      for ( ; ; ) {
> +        err = VLC_EGENERIC;
> +        if ( 0 > pool_add_exts(&pool,aud_exts,FILEEXT_AUDIO))    break;
> +        if ( 0 > pool_add_exts(&pool,vid_exts,FILEEXT_VIDEO))    break;
> +        if ( 0 > pool_add_exts(&pool,pls_exts,FILEEXT_PLAYLIST)) break;
> +        if ( 0 > pool_add_exts(&pool,sub_exts,FILEEXT_SUBTITLE)) break;
> +        err = VLC_SUCCESS;
> +        break;
>
> It seems that you really don't like goto :D

I don't really care :-) . It's sometime convenient to do as such.

>
> +char * fileext_Get( int types )
> +{
> +   return valid_type(types)
> +      ? pool_get(&pool, types)
> +      : 0
> +      ;
> +}
>
> Please use NULL rather than 0. We don't like implicit cast warnings.

I have never seen any cast warning for 0 to pointer converson in C
whatever -Wall -pedantic -std= combinaison I ever used. But I don't
mind using NULL.

-- 
B.