[vlc-devel] [Patch] Adding an helper to convert iso8601 durations to seconds
Hugo Beauzée-Luyssen
beauze.h at gmail.com
Tue Dec 6 15:41:49 CET 2011
On Tue, Dec 6, 2011 at 3:02 PM, Hugo Beauzée-Luyssen <beauze.h at gmail.com> wrote:
> 2011/12/6 Rémi Denis-Courmont <remi at remlab.net>:
>> 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.
>>
>
> Okey, I thought that was ok but I will fix it.
>
Should be better, but to be honest I find it harder to read.
>> Also, the test case name is very poor IMHO.
>>
>
> It is. Will add some.
>
I've added three tests, though I think that's enough to test the basic
behaviour.
Regards,
--
Hugo Beauzée-Luyssen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-strings-Adding-an-helper-to-convert-iso8601-duration.patch
Type: text/x-patch
Size: 3112 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20111206/44f08caf/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-tests-Running-new-strings-tests-in-the-test-suite.patch
Type: text/x-patch
Size: 3399 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20111206/44f08caf/attachment-0001.bin>
More information about the vlc-devel
mailing list