[FFmpeg-devel] [PATCH] Implement the function cmdutils.c:parse_int_or_die

Stefano Sabatini stefano.sabatini-lala
Tue Feb 19 13:07:21 CET 2008


On date Monday 2008-02-18 12:07:01 -0500, D. Hugh Redelmeier encoded:
> | From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
[...]
> | > I'm not an expert in floating point but this concerns me:
> | > +        } else if (errno == ERANGE || d < -INFINITY || d > INFINITY) {
> | > If we are assuming that the C implementation includes the IEC 559
> | > support, then I think that the best test would be:
> | > +        } else if (errno == ERANGE || !isfinite(d)) {
> | > This rejects NaNs.  It also handles the afine case (I think that is
> | > the right term).
> | 
> | Fixed in a different way, since now I'm expecting a double than there
> | is no need to check for float limits.
> 
> I still think that you should be testing that the result passes the
> isfinite() test.  According to strtod documentation, you could be
> getting infinities or NaNs and I expect that that is bad news.
> (I think that isfinite is an optional part of the C standard, but then
> I think INFINITY is too.  I have not checked this.)

According to the Gnu libc manual, both INFINITY and the macro isfinite()
are part of C99, and FFmpeg is implemented with C99, so I think it's
just OK to use them.

On the other end NaN is a Gnu extension, so we can't rely on that,
while (strangely enough) the isnan() macro seems to be part of ISO
C99.

I'm OK to use the isnan() check, for what regards the acceptance of
infinite values see below.

> I have NOT looked at the codebase that these patches are intended for.
> How frequently will parse_double_or_die be called with interesting
> bounds (i.e. other than [-INFINITY, INFINITY])?  My hunch is that it
> would be uncommon (based on no data!).  I know that the range check
> makes sense for integers but it might not for floats.

I have no strong opinion on this, but from a purely mathematically
point of view if we're saying that a double may be e.g. say <= inf
then we should accept also the inf value, so I think the function
shouldn't exit if the value is "inf", and we could have a variable
which for some strange reason accept the inf or -inf value.

Eventually we could add a flag to tell parse_double_or_die() to accept
only finite values; another idea could be to have a flag for
inclusivity/exclusivity in the range check.

> In the integral case, smaller types have smaller ranges.  In floating
> point, smaller types have smaller precision.  Kind of different.
> 
> | I struggled to take into consideration yours and Michael's
> | suggestions, hope the attached patch would be another step in the
> | right direction, blame me if it isn't ;-).
> 
> Sorry to be quibbling so much.  I do think that you are converging.
> Your patch is much better than the status quo.
> 
> + * @param context the context of the value to be setted (e.g. the
>                                                   ######
> 
> The correct English word is "set".  Silly language.

Yes I agree, shame on its designers ;-), fixed.

Regards.
-- 
Stefano Sabatini
Linux user number 337176 (see http://counter.li.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: implement-number-parsing-functions-01.patch
Type: text/x-diff
Size: 3911 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080219/3f6c695d/attachment.patch>



More information about the ffmpeg-devel mailing list