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

[Michael Niedermayer]

On Sun, Feb 17, 2008 at 05:08:46PM -0500, D. Hugh Redelmeier wrote:
> | 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.

parse_options() for example could maybe benefit from a single function.


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

This parses command line options and the range of valid values is not
something which should depend on LLONG_MAX. Whatever the allowed range it
should be allowed independant of LLONG_MAX.


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

If max / min where set to values outside the int64_t range then the code
would fail fatally on a system where long long has just int64_t range (which
frankly are all practical systems).


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

There is no reason why non finite values would be invalid inputs.


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

Tabs are not allowed in svn
The errno thing should disapear
the != '\0' is also useless
except that your code looks better then the last patch

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080219/cfd0b693/attachment.pgp>