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

[Michael Niedermayer]

On Wed, Feb 20, 2008 at 01:51:22AM -0500, D. Hugh Redelmeier wrote:
> | 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.

No, double bounds are fine.


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

no

d= strtod()
if(type=integer && (int64_t)d != d)
    error
if(d<min || d>max)
    error
...


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

That is not true. Quoting the C spec:
--------
 minimum value for an object of type long long int
                    -9223372036854775807 // -(2^63 - 1)
 LLONG_MIN
--------
 minimum values of exact-width signed integer types
                                           exactly -(2^(N-1) )
 INTN_MIN
--------


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

This really is obvious isnt it?
Lets assume ffmpeg used -9223372036854775809, it would fail if the system
doesnt support such a value -> we can only use values guranteed to be
supported by ISO C. And ffmpeg should behave as identical as possible
on different platforms thus use of LLONG_MIN as min of a command line
option is highly undesireable.


> 
> | There is no reason why non finite values would be invalid inputs.
> 
> Is there a use for this currently?

I dont know


>  Does all the code correctly handle
> the non-finite cases?

I dont know, i also dont know if all the code handles 0.0 or 1.0 or
1.23 correctly.


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

No, not real ones.


> 
> | the != '\0' is also useless
> 
> I think that it makes the code a lot easier to read.

Peoples opinions differ. Ours is != 0 has to be removed.


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

spare me of these please


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

The worst form of inequality is to try to make unequal things equal.
-- 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/20080220/a820b5bb/attachment.pgp>