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

[Stefano Sabatini]

On date Sunday 2008-02-17 17:08:46 -0500, D. Hugh Redelmeier encoded:
> | From: Stefano Sabatini <stefano.sabatini-lala at poste.it>
> 
> | Attached patch proposes a solution which implements MN suggestion, for
> | what regards the double to int conversion we could simply split the
> | function int three different ones, like:
> | 
> | int parse_int_or_die(...)
> | int64_t parse_int64_or_die(...)
> | float parse_float_or_die(...)
> 
> [Sorry if I'm charging in like a bull in a china shop.  I have not
> read any ffmpeg code except for the patches I am critiquing.  I have
> not even participated in the community to learn who is who.]
> 
> This is an example where I would expect separate functions to be
> superior.  I am guessing that in most call sites, the case is known
> (i.e. the type argument is a static constant).  At a minimum I would
> use separate functions for the float and integral cases.

Yes, and this also fixes the controversial double to int conversion
problem.
 
> Matter of taste: int64_t should only be used where the problem
> dictates it.  long int or long long int are more abstract.  Of course
> there is the problem that you don't know exactly what you are getting.
> The C language really isn't very good in this area.  It is true that
> long long can hold any int64_t value but the reverse may not be the
> case.
> 
> If the function is just handling integers, then perhaps it need not
> distinguish OPT_INT and OPT_INT64.  The relevant difference will be
> captured in the min and max arguments.  Unfortunately, C does not tell
> you that int64_t can handle all values of type long.  The type that we
> can be sure will hold all values of int64_t and of long would be long
> long.  Note that long long may not hold all values of uint64_t (if
> that matters).
> 
> +  fprintf(stderr, "Value for %s has be <= to %lld and <= %lld: %s\n", context, (long long int)min, (long long int)max, numstr);
> 
> The sense of min is reversed:  >= is what you want.
> The grammar needs touching up too.
> 
> I think a message like this might be clearer:
> 
> + 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.
 
> (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.

Ditto for the parse_double_or_die function.

> 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.
 
> This UNTESTED routine handles uint64_t, int, long, and long long.  But
> not floating point.  It is extracted from Sefano's code.  The double
> analogue is needed too.
> 
> long long parse_integer_or_die(const char *context, const char *numstr, long long min, long long max)
> {
>     long long int lli;
>     char *tail;
> 
>     /* errno could have been set by previous library calls, so reset it */
>     errno = 0;
>     lli = strtoll(numstr, &tail, 10);
>     if (*tail != '\0') {
> 	fprintf(stderr, "Expected integer for %s but found: %s\n", context, numstr);
> 	exit(1);
>     } else if (lli < min || lli > max) {
>         fprintf(stderr, "The value for %s must be between %lld and %lld but you used %lld\n",
> 	    context, min, max, lli);
> 	exit(1);
>     }
>     return lli;
> }

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

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-00.patch
Type: text/x-diff
Size: 3905 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080218/9e6a603b/attachment.patch>