[MPlayer-dev-eng] [PATCH] ftp charset selection support

[Andrew Savchenko]

Hi,

On Sunday 20 April 2008 00:31, Reimar Döffinger wrote:
> On Sat, Apr 19, 2008 at 11:18:42PM +0400, Andrew Savchenko wrote:
> > --- mplayer_base/mp_msg.h	2008-03-08 02:46:16.000000000 +0300
> > +++ mplayer/mp_msg.h	2008-04-19 21:47:21.000000000 +0400
> > @@ -127,6 +127,21 @@
> >  #   endif
> >  #endif /* __GNUC__ */
> >
> > -const char* filename_recode(const char* filename);
> > +#ifdef USE_ICONV
> > +#include <iconv.h>
> > +
> > +/* Recode filename from <tocode> to <fromcode> charset.
> > + *
> > + * *icnv -- iconv handler, unique for caller;
> > + * *buf  -- buffer for recoded result, must be allocated
> > outside, + *          but may be grown inside the function;
> > + * *buf_size -- size of buffer to use. */
> > +const char* mp_recode_to(iconv_t* const icnv,
> > +    const char* const filename,
> > +    const char* const tocode, const char* const fromcode,
> > +    char** const buf, size_t* const buf_size);
> > +#endif /* USE_ICONV */
> > +
> > +const char* filename_recode(const char* const filename);
>
> I see now point in about half of those consts, they IMO just
> obfuscate the code, 

They are harmless, besides const qualifier protects from some 
development errors and may help compiler to optimize code or may 
not, but nevertheless they change nothing in the worst case. The 
only drawback is a little code readability decreasing, so perhaps 
they are worth to be used.

> and esp. the declaration of filename_recode 
> should not change.

I agree with you here, if stable function interface may be 
untouched, it should be untouched.

> > --- mplayer_base/mp_msg.c	2008-04-12 20:24:22.000000000 +0400
> > +++ mplayer/mp_msg.c	2008-04-19 21:46:20.000000000 +0400
> > @@ -31,38 +31,74 @@
> >  int mp_msg_module = 0;
> >  #ifdef USE_ICONV
> >  char *mp_msg_charset = NULL;
> > -static char *old_charset = NULL;
> > -static iconv_t msgiconv;
> > -#endif
> > -
> > -const char* filename_recode(const char* filename)
> > +char *old_charset = NULL;
> > +iconv_t msgiconv;
> > +iconv_t inv_msgiconv = (iconv_t)(-1);
>
> Why removing the static? 

They are in global scope, so static by default. Static declarations 
are absolutely useless here, so they was removed. Well, if you 
insist I'll keep them, anyway they change nothing.

> Why moving those variables out instead 
> of leaving it in the filename_recode function?

No reason ;-/, moved it back.

> > +char *recoded_filename;
> > +size_t recoded_filename_size = MSGSIZE_MAX;
> > +
> > +// recode filename from <tocode> to <fromcode> charset
> > +const char* mp_recode_to(iconv_t* const icnv,
> > +    const char* const filename,
> > +    const char* const tocode, const char* const fromcode,
> > +    char** const buf, size_t* const buf_size)
> >  {
> > -#if !defined(USE_ICONV) || !defined(MSG_CHARSET)
> > -    return filename;
> > -#else
>
> And e.g. this (and a bit of other code) still exists after the
> patch, by simply moving the new filename_recode function before
> the mp_recode_to quite a bit of diff should disappear and make
> this much, much easier to understand.

Done, this patch was really hard to read.

Here is the updated patch for mp_msg.[ch]. 
The second patch is unchanged.

With best regards,
Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ftp-charset-recode.patch
Type: text/x-diff
Size: 4264 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080420/a9bcfa3a/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ftp-charset.patch
Type: text/x-diff
Size: 2912 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080420/a9bcfa3a/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20080420/a9bcfa3a/attachment.pgp>