This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA_v2 1/8] Add helper functions check_for_flags and check_for_flags_vqcs
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