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

D. Hugh Redelmeier hugh
Mon Feb 18 18:07:01 CET 2008


| From: Stefano Sabatini <stefano.sabatini-lala at poste.it>

| > + fprintf(stderr, "The value for %s must be between %lld and %lld but you used %lld\n",
| > +	context, (long long int)min, (long long int)max, lli);
| 
| Yes, only I prefer to use the more explicit >= and <= symbols.

That is reasonable.

| > (I chose to print the number we extracted rather than the string we
| > were given since that may be slightly more informative.)
| 
| I'm also trying to manage the overflow case in that block, and in case
| of overflow the variable lli could be setted to 0, so I'd prefer to
| use numstr instead.

Right.

| > 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.)

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.

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.




More information about the ffmpeg-devel mailing list