[vlc-devel] [Patch] Adding an helper to convert iso8601 durations to seconds

[Rémi Denis-Courmont]

Le mardi 6 décembre 2011 14:11:10 Hugo Beauzée-Luyssen, vous avez écrit :
> On Wed, Nov 30, 2011 at 4:41 PM, Hugo Beauzée-Luyssen
> 
> <beauze.h at gmail.com> wrote:
> > On Wed, Nov 30, 2011 at 2:45 PM, Hugo Beauzée-Luyssen
> > 
> > <beauze.h at gmail.com> wrote:
> >> On Wed, Nov 30, 2011 at 2:08 PM, Tobias Güntner <fatbull at web.de> wrote:
> >>> Am 30.11.2011 11:05, schrieb Hugo Beauzée-Luyssen:
> >>>> +/*
> >>>> +  Decodes a duration as defined by ISO 8601
> >>>> +  http://en.wikipedia.org/wiki/ISO_8601#Durations
> >>>> +  @param str A null-terminated string to convert
> >>> 
> >>> Perhaps a short example of a valid input string would be useful, so
> >>> readers can see at first glance what kind of data this function
> >>> expects, without looking it up.
> >> 
> >> Yep I'm going to write some tests in a this afternoon or tomorrow, but
> >> I could add a link to the format description and an exemple in the
> >> comments.
> >> 
> >>>> +  @return: The duration in seconds. 0 if an error occured.
> >>> 
> >>> This function usually does not return 0 if the input is invalid.
> >> 
> >> Yep i'm going to change the return type as a signed int 64, so -1 is an
> >> error.
> >> 
> >>>> + */
> >>>> +uint64_t str_iso8601_duration_to_seconds( const char *psz_duration )
> >>>> +{
> >>>> +    bool        timeDesignatorReached = false;
> >>>> +    double      mul = 0;
> >>> 
> >>> I think an integer should be fine.
> >> 
> >> Well in the exemple file I've got
> >> (http://www-itec.uni-klu.ac.at/ftp/datasets/mmsys12/Selected_VLC_Plugin_
> >> MPDs/BigBuckBunny_15s_DASH_VLC_Compatible_4-Representations_720p.mpd)
> >> there are some decimal values, so I try to reduce the loss of
> >> precision when converting by using doubles.
> >> 
> >>>> +    mtime_t     res = 0;
> >>> 
> >>> Shouldn't this type and the return type be the same?
> >> 
> >> I was using mtime_t at first, it seems I forgot to change this one, my
> >> bad.
> >> 
> >> Since the line below is a direct pointer dereferencement without any
> >> prior check, this was intended, but a check could be added.
> >> 
> >>>> +
> >>>> +    if ( *psz_duration++ != 'P' )
> >>>> +        return 0;
> >>>> +    for ( const char* nptr = psz_duration; *psz_duration;
> >>>> ++psz_duration ) +    {
> >>>> +        switch( *psz_duration )
> >>>> +        {
> >>>> +            case 'M':
> >>>> +            {
> >>> 
> >>> I find this a little confusing because letters normally follow numbers
> >>> in the input, yet you somehow parse letters first. ;)
> >> 
> >> That is absolutely right...
> >> 
> >>>> +                //M can mean month or minutes, if the 'T' flag has
> >>>> been reached. +                //We don't handle months though.
> >>>> +                if ( timeDesignatorReached == true )
> >>>> +                    mul = 60.0;
> >>>> +                break ;
> >>>> +            }
> >>>> +            case 'Y':
> >>>> +            case 'W':
> >>>> +                break ; //Don't handle this duration.
> >>> 
> >>> Maybe return an error here?
> >> 
> >> I don't really know... warning should be mandatory, an error may be to
> >> agressive...
> >> 
> >>>> +            case 'D':
> >>>> +                mul = 86400.0;
> >>>> +                break ;
> >>>> +            case 'T':
> >>>> +                timeDesignatorReached = true;
> >>>> +                break ;
> >>>> +            case 'H':
> >>>> +                mul = 3600.0;
> >>>> +                break ;
> >>>> +            case 'S':
> >>>> +                mul = 1.0;
> >>>> +                break ;
> >>>> +            default:
> >>>> +                continue ;
> >>>> +        }
> >>>> +        //It means we went trough some digits. So we must compute a
> >>>> number +        if ( nptr != psz_duration )
> >>>> +        {
> >>>> +            double tmp = strtold( nptr, NULL );
> >>> 
> >>> I think you can achieve a slightly cleaner design if you utilize the
> >>> second parameter of that function. The strtoX functions store the
> >>> address of the first invalid character in there, so all you have to do
> >> 
> >>> is basically:
> >> Yep since I'm doing it in the wrong way, using the second parameter
> >> makes a lot of sense now :D
> >> 
> >>> 1. Call strtoul(psz_duration, &endptr, 10)
> >>> 2. Check the character at *endptr
> >>> 3. Repeat with psz_duration = endptr + 1
> >>> 
> >>>> +            res += (uint64_t)(tmp * mul);
> >>>> +            nptr = psz_duration;
> >>>> +        }
> >>>> +        nptr++;
> >>>> +    }
> >>>> +    return res;
> >>>> +}
> >>> 
> >>> Regards,
> >>> Tobias
> >> 
> >> Thanks for the review!
> >> Regards,
> > 
> > Here is a new version, along with some unit tests.
> > 
> > Regards,
> 
> Hi,
> 
> Could somebody review these patches?


+    if ( *psz_duration++ != 'P' )
+        return -1;

With *three* independent operators (dereference, increment and inequality) on 
psz_duration in a single expression, you really need more explicit parenthesis 
IMHO.

Also, the test case name is very poor IMHO.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis