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

[Stefano Sabatini]

On date Tuesday 2008-02-19 13:07:21 +0100, Stefano Sabatini encoded:
> 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.

[...] 
> @@ -104,11 +143,11 @@
>              } else if (po->flags & OPT_BOOL) {
>                  *po->u.int_arg = 1;
>              } else if (po->flags & OPT_INT) {
> -                *po->u.int_arg = atoi(arg);
> +                *po->u.int_arg = parse_integer_or_die(opt+1, arg, INT_MIN, INT_MAX);
>              } else if (po->flags & OPT_INT64) {
> -                *po->u.int64_arg = strtoll(arg, (char **)NULL, 10);
> +                *po->u.int64_arg = parse_integer_or_die(opt+1, arg, INT64_MIN, INT64_MAX);
>              } else if (po->flags & OPT_FLOAT) {
> -                *po->u.float_arg = atof(arg);
> +                *po->u.float_arg = parse_double_or_die(opt+1, arg, -INFINITY, INFINITY);
>              } else if (po->flags & OPT_FUNC2) {
>                  if(po->u.func2_arg(opt+1, arg)<0)
>                      goto unknown_opt;

Sorry this hunk belongs to another patch.

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-02.patch
Type: text/x-diff
Size: 3091 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080219/011ece77/attachment.patch>