[vlc-devel] [PATCH] cmdline enhancements

[Lyndon Brown]

On Fri, 2020-09-18 at 20:20 +0200, Alexandre Janniaux wrote:
> Hi
> 
> In patch: cmdline: simplify, use memset
> 
> > -    i_shortopts = 0;
> > -    for( i_index = 0; i_index < 256; i_index++ )
> > -    {
> > -        pp_shortopts[i_index] = NULL;
> > -    }
> > +    memset(&pp_shortopts, 0, 256 * sizeof(pp_shortopts[0]));
> 
> You should not use memset to initialize pointers.

Fair enough. I guess at the time I was assuming that a NULL pointer
always equalled zero, which, looking up why you might consider this to
be a problem, I see is actually not something that should not really be
taken for granted and I presume is the basis of your comment.

I've replaced that commit with an alternative using simple array
initialisation, which is suggested at [1] is a valid way to null-
initialise a pointer array, and a much better solution anyway. :)

[1]: 
https://stackoverflow.com/questions/12735988/what-is-the-correct-way-to-memset-an-array-of-pointers

> In patch: cmdline: use color error/warn labels
> 
> Why not just check for --color/--no-color in the options first
> before parsing them? It's not like there would be 3000 of them.
> If I'm not wrong this would also be called on libvlc app too.

(and --nocolor (note the missing dash), which i suspect you may not
know is supported, and is a whole other pending discussion :p )

one objection to doing that, which was certainly something considered,
is the correctness of interpreting options. although scanning for an
argument of "--color" might work fine being pragmatic, strictly
speaking you cannot know whether any given argument is an option, a
non-option, or an option's value, without processing the set of
arguments before it. one easy to miss complication to consider for
instance is the "--" "early terminator" argument, which means treat all
subsequent options as non-options, never mind the (okay unlikely)
possibility of a user specifying "--color"/"--no-color"/"--" as a value
to another option, which would be a surprising quirk to users that this
is wrongly interpreted.

so we'd at least end up with a block of logic scanning for "--color" to
turn it on, "--no-color" and "--nocolor" to turn it off, and "--" to
stop processing, and ignoring mis-interpretation of any of these being
specified as values of other options (even if unlikely to be done in
legit normal usage). is that really worth it considering the
alternative, and considering it only concerns use of colour in option
parsing warnings/errors.

i know that the vlc app does a similar things looking for a few options
without full knowledge of the option set passing through it to the
core, and thus could make interpretation mistakes, but that's less easy
to avoid.

since colour highlighting of errors/warnings is a very useful thing,
and considering this is correctly only done anyway when output is
connected to a tty (rather than redirected to another app or a file),
it's not really problematic to force colour temporarily on users, for
the purposes of reporting cmdline parsing problems, and it's a more
simple solution.

i thoroughly considered the various angles, and felt most comfortable
with the setup implemented.

> Regards,
> --
> Alexandre Janniaux
> Videolabs
> 
> On Thu, Sep 17, 2020 at 11:41:09PM +0100, Lyndon Brown wrote:
> > set of 14 patches enhancing cmdline experience:
> > https://code.videolan.org/jnqnfe/vlc/-/tree/cmdline
> > 
> > notable highlights:
> >  - adds unknown long option suggestion matching (i.e. when
> > possible,
> > alongside an unknown long option error, a suggestion is given of
> > the
> > available option that most closely matches it, helping users when
> > they
> > make typos).
> >  - colour error/warn labels.
> >  - distinction made between unknown option and missing data value
> > error
> > conditions.
> >  - other minor fixes/improvements.
> > 
> > (had to bundle the FLT_MIN fix just separately submitted).
> > 
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel