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