[vlc-devel] [PATCH] Rewrites asx file support using vlc_xml API

[Jean-Baptiste Kempf]

On 29 Sep, Reka Inovan wrote :
> +    char *s = NULL;

avoid one-char variables.

> +    const char *psz_node=NULL;
> +    const char *psz_txt=NULL;

use spaces around "="

> +    do
> +    {
> +        psz_txt = xml_ReaderNextAttr( p_xml_reader, &psz_node );
> +    }
> +    while( strncasecmp( psz_txt, "VALUE", 5 ) );
Add a newline here.

> +    result = result*1000;
> +    s = SkipBlanks(s, end-s);
> +    if( *s == ':' )
> +    {
> +        ++s;
> +        s = SkipBlanks(s, end-s);
> +        val = 0;
> +        while( (s < end) && isdigit((unsigned char)*s) )

 Why cast to unsigned?

> +        {
> +            int newval = val*10 + (*s - '0');

Missing spaces.

> +            if( newval < val )
> +            {
> +                // overflow
> +                val = 0;
> +                break;
> +            }
> +            val = newval;
> +            ++s;
> +        }
> +        result += val;
> +    }
> +    result = result*1000;

> +    free( (void*) s_init );

Why not free directly?


> +static void ReadElement( xml_reader_t *p_xml_reader, const char **ppsz_txt )
> +{
> +    const char *psz_node = NULL;
> +    xml_ReaderNextNode( p_xml_reader, &psz_node );
> +    if( *ppsz_txt ) free( (void*) *ppsz_txt );

free( NULL ) is valid.

> +    int i_options;
> +    mtime_t i_start =0;
> +    mtime_t i_duration=0;

missing spaces around =

> -            else if( !strncasecmp( psz_parse, "<BASE ", 6 ) )
> +            if( !strncasecmp( psz_node, "ABSTRACT", 8 ) )
> +                ReadElement( p_xml_reader, &psz_description );
> +            // Time related element
> +            if( !strncasecmp( psz_node, "DURATION", 8 ) )
> +                i_duration = ParseTime( p_xml_reader );
> +            if( !strncasecmp( psz_node, "STARTTIME", 9 ) )
> +                i_start = ParseTime( p_xml_reader );
> +            // Href element
> +            if( !strncasecmp( psz_node, "REF", 3 ) )

Not sure if those comments are useful.

> +                psz_mrl = ProcessMRL( psz_href, psz_prefix );
> +                //Add Time information

Add new line above that.

> +    if( psz_href ) free( (void*) psz_href );
> +    if( psz_title ) free( (void*) psz_title );
> +    if( psz_artist ) free( (void*) psz_artist );
> +    if( psz_copyright ) free( (void*) psz_copyright );
> +    if( psz_moreinfo ) free( (void*) psz_moreinfo );
> +    if( psz_description ) free( (void*) psz_description );

free ( NULL ) is fine.

Seeing that you basically rewrote the file, maybe sending a the new file
for review would help :)

Best regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device