Re: [PATCH v2 3/6] parse-options: factor out register_abbrev() and struct parsed_option
- From
Josh Steadmon <steadmon@google.com>
- Date
- Mar 12, 2024, 21:43 UTC
- Message-ID
- <ZfDMhJ4r5g-g-B3n@google.com>
- In-Reply-To
- <20240303121944.20627-4-l.s.r@web.de>
I found this change to be hard to follow, although I'm not sure anything actually needs to be changed. Thinking aloud below, apologies for being verbose.
On 2024.03.03 13:19, René Scharfe wrote:
Show 25 quoted lines
> Add a function, register_abbrev(), for storing the necessary details for
> remembering an abbreviated and thus potentially ambiguous option. Call
> it instead of sharing the code using goto, to make the control flow more
> explicit.
>
> Conveniently collect these details in the new struct parsed_option to
> reduce the number of necessary function arguments.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
> parse-options.c | 83 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 49 insertions(+), 34 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 056c6b30e9..398ebaef14 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -350,14 +350,40 @@ static int is_alias(struct parse_opt_ctx_t *ctx,
> return 0;
> }
>
> +struct parsed_option {
> + const struct option *option;
> + enum opt_parsed flags;
> +};We're bundling up the state for previously-examined options that "arg" might expand to. Looks good.
Show 5 quoted lines
> +static void register_abbrev(struct parse_opt_ctx_t *p,
> + const struct option *option, enum opt_parsed flags,
> + struct parsed_option *abbrev,
> + struct parsed_option *ambiguous)
> +{Here we're defining a function to separate out the logic that we previously jumped to with "goto is_abbreviated;". Looks good.
> + if (p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT) > + return;
This is the (negation of the) allow_abbrev condition that was removed below. Looks good.
Show 10 quoted lines
> + if (abbrev->option &&
> + !is_alias(p, abbrev->option, option)) {
> + /*
> + * If this is abbreviated, it is
> + * ambiguous. So when there is no
> + * exact match later, we need to
> + * error out.
> + */
> + ambiguous->option = abbrev->option;
> + ambiguous->flags = abbrev->flags;Couldn't this just be "*ambiguous = *abbrev;" ?
> + } > + abbrev->option = option; > + abbrev->flags = flags; > +}
So, we've found a candidate that matches our abbreviation. If this is the second (or later) candidate, then we've got an ambiguous abbreviation, which we'll need to warn about later. Since we're overwriting both "ambiguous" and "abbrev", we'll only refer to the last two candidates seen, but that seems fine.
Show 36 quoted lines
> static enum parse_opt_result parse_long_opt(
> struct parse_opt_ctx_t *p, const char *arg,
> const struct option *options)
> {
> const char *arg_end = strchrnul(arg, '=');
> - const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> - enum opt_parsed abbrev_flags = OPT_LONG, ambiguous_flags = OPT_LONG;
> - int allow_abbrev = !(p->flags & PARSE_OPT_KEEP_UNKNOWN_OPT);
> + struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG };
> + struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG };
>
> for (; options->type != OPTION_END; options++) {
> const char *rest, *long_name = options->long_name;
> @@ -377,31 +403,20 @@ static enum parse_opt_result parse_long_opt(
> rest = NULL;
> if (!rest) {
> /* abbreviated? */
> - if (allow_abbrev &&
> - !strncmp(long_name, arg, arg_end - arg)) {
> -is_abbreviated:
> - if (abbrev_option &&
> - !is_alias(p, abbrev_option, options)) {
> - /*
> - * If this is abbreviated, it is
> - * ambiguous. So when there is no
> - * exact match later, we need to
> - * error out.
> - */
> - ambiguous_option = abbrev_option;
> - ambiguous_flags = abbrev_flags;
> - }
> - abbrev_option = options;
> - abbrev_flags = flags ^ opt_flags;
> + if (!strncmp(long_name, arg, arg_end - arg)) {
> + register_abbrev(p, options, flags ^ opt_flags,
> + &abbrev, &ambiguous);This part I found hard to follow; the change itself is a simple replacement of the original logic with our new function, but I don't understand the original logic in the first place. Why do we XOR flags and opt_flags here?
These are both bitflags which can hold a combination of OPT_SHORT and/or OPT_UNSET (i.e., we've passed --no-foo). Since we're only looking at args that we already know are in long form, the only one we should need to worry about is whether OPT_UNSET is true or false. And indeed, we never set OPT_SHORT in this function.
IIUC, "flags" corresponds to "arg", the flag we're trying to parse, and "opt_flags" corresponds to "options", the current item in the list of all options that we're trying to match against.
So OPT_UNSET will be true in "flags" if either "arg" starts with "no-" or if it is a prefix of "no-".
OPT_UNSET will be true in "opt_flags" if all of the following are true: * "arg" does not start with "no-" * PARSE_OPT_NONEG is not set in options->flags * options->long_name starts with "no-"
So OPT_UNSET can never be set at the same time in both "flags" and "opt_flags", and thus the XOR makes a bit more sense: either the argument we're parsing or the canonical form of the candidate that we're matching against is negated. We don't care which one, but we need to know about the negation later, to either set the value properly, or to report potential ambiguities with the "no-" prefix.
Show 13 quoted lines
> continue;
> }
> /* negation allowed? */
> if (options->flags & PARSE_OPT_NONEG)
> continue;
> /* negated and abbreviated very much? */
> - if (allow_abbrev && starts_with("no-", arg)) {
> + if (starts_with("no-", arg)) {
> flags |= OPT_UNSET;
> - goto is_abbreviated;
> + register_abbrev(p, options, flags ^ opt_flags,
> + &abbrev, &ambiguous);
> + continue;This is a slight change: we might set OPT_UNSET in flags where before we were prevented by the "allow_abbrev" condition, but register_abbrev will still be a no-op in that case, so I don't think this changes any behavior.
All the other changes in this patch are straightforward. So, despite having to puzzle out the original logic, everything here looks good.