[MPlayer-dev-eng] Re: [PATCH]Unicode support for ASF demuxer

[Reimar Döffinger]

Hello,
On Tue, Nov 28, 2006 at 09:31:27AM +0800, Zuxy Meng wrote:
> 2006/11/28, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> >On Tue, Nov 28, 2006 at 12:03:27AM +0800, Zuxy Meng wrote:
> >> -// the variable string is modify in this function
> >> -void pack_asf_string(char* string, int length) {
> >> -  int i,j;
> >> -  if( string==NULL ) return;
> >> -  for( i=0, j=0; i<length && string[i]!='\0'; i+=2, j++) {
> >> -    string[j]=string[i];
> >> +static char* get_ucs2str(const uint16_t* inbuf, uint16_t inlen, char* 
> >outbuf,
> >> +    uint16_t outlen)
> >
> >Due to the way it is used, IMO remove the outbuf and outlen and just do
> >the malloc in this function.
> 
> When should the malloc'ed be freed then? Do we need a file-scope
> static pointer var or something?

At the same place and in the same way as it is done now, just merge the
malloc and the call to this function.

> >> -            print_asf_string(" Title: ", string, contenth->title_size);
> >> -       else
> >> -         pack_asf_string(string, contenth->title_size);
> >> +         mp_msg(MSGT_HEADER,MSGL_V,"%s%s\n", " Title: ", string);
> >
> >" Title: %s\n" seems to make a lot more sense to me... There are more
> >cases like that below.
> 
> Good! I wasn't sure if changing old code in a somewhat unrelated way
> is acceptable...

The goal is patch readability, if anything you would have had to leave
the print_asf_string function there, but this hardly makes any sense, so
just do it properly IMO, that version would just make both the patch and
the resulting code harder to understand.

Greetings,
Reimar Döffinger