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

[Rémi Denis-Courmont]

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.

+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.


+  /* 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.

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).

+   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.

+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).

+/* MUST BE LOCK

s/LOCK/LOCKED/

+      fileext = realloc(pool->fileext, newmax * sizeof(fileext_t));

Potential integer overflow? Maybe not really an issue.

+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.

+      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

+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.

Regards,

-- 
Rémi Denis-Courmont
http://git.remlab.net/cgi-bin/gitweb.cgi?p=vlc-courmisch.git;a=summary