Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote:
> Hi Philippe,
> 
> Been taking a better look at this, finally.
Thanks for the review, I will handle all the comments,
I have some feedback/questions on a few of them below.


> > 	* cli-utils.c (number_or_range_parser::get_number): Only handle
> > 	numbers or convenience var as numbers.
> > 	(check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> > 	* cli-utils.h (check_for_flags): New function.
> > 	(check_for_flags_vqcs): New function.
> 
> I'm not super happy with this design, because first it is
> still at places biased toward "flags" instead of "command
> options", and most importantly because it doesn't seem to
> make it easy to add further options to commands that
> use check_for_flags_vqcs, without the artificial limitation
> of requiring all vqcs flags specified together.  E.g., what if we want to
> add an option like "-foo NUM" to "thread apply" for example.

Yes I agree, the current way to give the vqcs option is too unflexible,
so I will rework based on the iterative function you suggest below.

Just one clarification: I assume that by 'at places biased toward "flags"',
you mean that the names should be 'check_for_options_vqcs' ?
Otherwise, can you explain what you mean with bias ?

> 
> I'd seem to me that at least an iterative function which
> could be interleaved with other options would be better.
> Something like:
> 
> struct vqcs_flags
> {
>   /* Number of times -c was seen.  */
>   int c = 0;
> 
>   /* Number of times -v was seen.  */
>   int v = 0;
> };
> 
> int parse_flags_vqcs (const char **args, vqcs_flags *flags);
> 
> and then
> 
>   vqcs_flags flags;
> 
>   while (*args != '\0')
>     {
>        if (parse_flags_vqcs (&args, &flags))
>          continue;
> 
>        /* check other options here.  error if unknown.  */
>     }
> 
> maybe even interleave the number-or-range parsing
> in that loop.
Probably that can be done, but isn't this a little bit cumbersome ?
E.g. it means the help 
  thread apply ID... [OPTIONS] COMMAND
will become something like
  thread apply OPTIONS_OR_ID... COMMAND
and then we have to explain what OPTIONS_OR_ID can be.

(and I guess such syntax might make the 'generalised option parser'
more difficult to implement/use : we better keep ID... as a 
'positional argument' for an easy conversion to an generalised
option/arg parser).

So, I am more keen to keep
   thread apply ID... [OPTIONS] COMMAND
(note: we can always change it in a backward compatible
way in the future if we really believe mixing OPTIONS and ID...
has a strong value).

+  res = check_for_flags (str, flags, flags_counts);
> > +  if (res == 0)
> > +    return 0;
> > +  if (res == -1)
> > +    error (_("%s only accepts flags %s given individually"),
> > +	   which_command, flags);
> 
> I think this error message might look a bit odd.  What does
> it really mean?
It catches the below erroneous case, but probably this will become
'unknown option -vc' when I do the iterative design:
  (gdb) thread apply all -vc p 1
  thread apply all only accepts flags vqcs given individually
  (gdb) 

(I think that the generalised option/arg parser you have prototyped
will be a nice help to have a consistent and easier to code
gdb command parsing :).

Thanks

Philippe