[FFmpeg-devel] [PATCH] ffprobe integration

[Michael Niedermayer]

On Mon, Feb 08, 2010 at 01:01:59AM +0100, Stefano Sabatini wrote:
> On date Sunday 2010-02-07 21:15:24 +0100, Michael Niedermayer encoded:
> > On Sun, Feb 07, 2010 at 03:55:24PM +0100, Stefano Sabatini wrote:
> > [...]
> > > +const Unit measure_units[UNIT_NB] = {
> > > +    [UNIT_NONE]            = { UNIT_NONE           , ""       },
> > > +    [UNIT_SECOND]          = { UNIT_SECOND         , "s"      },
> > > +    [UNIT_HERTZ]           = { UNIT_HERTZ          , "Hz"     },
> > > +    [UNIT_BIT]             = { UNIT_BIT            , "bit"    },
> > > +    [UNIT_BYTE]            = { UNIT_BYTE           , "byte"   },
> > > +    [UNIT_BYTE_PER_SECOND] = { UNIT_BYTE_PER_SECOND, "byte/s" },
> > > +    [UNIT_BIT_PER_SECOND]  = { UNIT_BIT_PER_SECOND , "bit/s"  }
> > > +};
> > 
> > as said this indirection over the table is silly
> > please use "byte" "bit" "Hz" ... dont invent a complicated system to
> > obfuscate constant strings
> 
> Passing a constant to a function rather than a string is both faster
> (no need to strcmp) and more robust (less likely to introduce a typo).

You are equally likely to introduce a typo in the table.
also there is
#define HZ_STR "Hz"
or even
const char *hz_str="Hz"


> 
> > > +
> > > +/** Print unit of measure. */
> > > +#define VALUE_STRING_USE_UNIT                      0x0001
> > > +
> > > +/** Print SI (binary or decimal) prefixes. */
> > > +#define VALUE_STRING_USE_PREFIX                    0x0002
> > > +
> > > +/** Use binary type prefixes, implies the use of prefixes */
> > > +#define VALUE_STRING_USE_BINARY_PREFIX             0x0004
> > > +
> > > +/** Force the use of binary prefixes with bytes measure units. */
> > > +#define VALUE_STRING_USE_BYTE_BINARY_PREFIX        0x0008
> > > +
> > > +/* Use sexagesimal time format HH:MM:SS.MICROSECONDS. */
> > > +#define VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT   0x0010
> > > +
> > > +struct flag_descriptor value_string_flag_descriptors[] = {
> > > +    { "unit"              , VALUE_STRING_USE_UNIT                    },
> > > +    { "prefix"            , VALUE_STRING_USE_PREFIX                  },
> > > +    { "byte_binary_prefix", VALUE_STRING_USE_BYTE_BINARY_PREFIX      },
> > > +    { "sexagesimal"       , VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT },
> > > +    { "pretty"            , VALUE_STRING_USE_UNIT                    |
> > > +                            VALUE_STRING_USE_PREFIX                  |
> > > +                            VALUE_STRING_USE_BYTE_BINARY_PREFIX      |
> > > +                            VALUE_STRING_USE_SEXAGESIMAL_TIME_FORMAT },
> > > +    { NULL }
> > > +};
> > > +
> > > +static int opt_format(const char *opt, const char *arg)
> > > +{
> > > +    return get_flags(&value_string_flags, value_string_flag_descriptors, arg);
> > > +}
> > 
> > please use
> > static int print_unit;
> > static int use_prefix;
> > ...
> > 
> > this is MUCH simpler
> 
> Yes but also less flexible.

i already have a java compiler i dont need another


> Also once given the get_flags function
> this requires also less code (just one line and the flags definitions)
> / options (just one compared to N).

huh?
you need 2 lines for what i suggest
static int print_unit;
and a single line in the cmdline parsing array that contains the string,
help text, default value, ...

your code does NOT work with less, you need at least the enum and
flag_descriptor
but its much messier, has no default, no help text


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- 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/20100208/a5d2fe4b/attachment.pgp>