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

[D. Hugh Redelmeier]

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

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

(I chose to print the number we extracted rather than the string we
were given since that may be slightly more informative.)

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


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;
}