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

[Pedro Alves <palves@redhat.com>]

On 06/14/2018 10:40 PM, Philippe Waroquiers wrote:
> 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 ?

Yeah, on second thought I think using "flags" for the function is
fine since it only handles flag-like options.  On my option-revamping
prototype I was calling those kind of options "switches" but I never
like that name.  "flags" sounds more usual.  So my concern is more
with the user-visible aspects.

>> maybe even interleave the number-or-range parsing
>> in that loop.
> Probably that can be done, but isn't this a little bit cumbersome ?

Maybe.

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

I guess we could also say that there's only one "thread apply"
command, and that is looks like this:

 thread apply [OPTION]... [ID... | all] COMMAND

and then specifying options after the ID list works
just because we don't care about order of optional vs
non-optional arguments.

But yeah, there's no need to go there.  Options after
ID is fine with me.

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

Agreed.

> 
> +  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 :).

I hope so.  :-)

Thanks,
Pedro Alves