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

[D. Hugh Redelmeier]

| From: Michael Niedermayer <michaelni at gmx.at>
| 
| 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.

If it is passing bounds, then it cannot benefit from unifying the
unification: different bounds must be specified for each type.

Even if bounds are not provided, then unification would combind two or
three cases in one caller at the expense of adding a switch and three
or four cases in the callee.  Not a good bargain in my opinion.

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

Sure, but I claim that any values of type int or int64_t will of
necessity (by way of guarantees in the C standard) be within the range
[LLONG_MIN, 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).

Why would the max / min values be set to such values?

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

Is there a use for this currently?  Does all the code correctly handle
the non-finite cases?

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

Good to know.

| The errno thing should disapear

Why?  I believe that it will catch real errors.

| the != '\0' is also useless

I think that it makes the code a lot easier to read.

For example, see
the 7th bullet point of section 9 of this coding standard:
  http://www.chris-lott.org/resources/cstyle/indhill-annot.html
and expanded in the section "Simple Statements" in
  http://www.chris-lott.org/resources/cstyle/indhill-cstyle.html

See the section "Expressions and Statements in
  http://www.jetcafe.org/jim/c-style.html

See http://www.cs.uiowa.edu/~jones/syssoft/style.html#bool