Pranit Bauva <pranit.ba...@gmail.com> writes:
> This change will not affect existing users of COUNTUP at all, as long as
> they use the initial value of 0 (or more).
>
> Force uses the "unspecified" value in conjunction with OPT__FORCE in
> builtin/clean.c in a different way to handle its config which
> munges the "-1" into 0 before we get to parse_options.
These two paragraphs leave the readers wondering what the conclusion
is. "it is OK as long as..." makes us wonder "so are there users
that do not satisfy that condition?" "in a different way" makes us
wonder "Does this change break builtin/clean.c because COUNTUP is
used in a different way?".
This change does not affect existing users of COUNTUP,
because they all use the initial value of 0 (or more).
Note that builtin/clean.c initializes the variable used with
OPT__FORCE (which uses OPT_COUNTUP()) to a negative value,
but it is set to either 0 or 1 by reading the configuration
before the code calls parse_options(), i.e. as far as
parse_options() is concerned, the initial value of the
variable is not negative.
perhaps?
> `OPT_COUNTUP(short, long, &int_var, description)`::
> Introduce a count-up option.
> - `int_var` is incremented on each use of `--option`, and
> - reset to zero with `--no-option`.
> + Each use of `--option` increments `int_var`, starting from zero
> + (even if initially negative), and `--no-option` resets it to
> + zero. To determine if `--option` or `--no-option` was set at
> + all, set `int_var` to a negative value, and if it is still
> + negative after parse_options(), then neither `--option` nor
> + `--no-option` was seen.
>
> `OPT_BIT(short, long, &int_var, description, mask)`::
> Introduce a boolean option.
> diff --git a/parse-options.c b/parse-options.c
> index 47a9192..86d30bd 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
> return 0;
>
> case OPTION_COUNTUP:
> - *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
> + if (unset)
> + *(int *)opt->value = 0;
> + else {
> + if (*(int *)opt->value < 0)
> + *(int *)opt->value = 0;
> + *(int *)opt->value += 1;
> + }
> return 0;
>
> case OPTION_SET_INT:
The new behaviour given by the patch looks very sensible, but I
wonder if this is easier to reason about:
case OPTION_COUNTUP:
+ if (*(int *)opt->value < 0)
+ *(int *)opt->value = 0;
*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
That is, upon hitting this case arm, we know that an explicit option
was given from the command line, so the "unspecified" initial value,
if it is still there, gets reset to 0, and after doing that, we just
do the usual thing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
